Fix #3314 - GBXML translate spaces etc even if Facility and/or Building aren't instantiated

src/gbxml/ForwardTranslator.cpp

@@ -188,10 +188,9 @@ namespace gbxml {
     gbXMLElement.append_attribute("version") = "6.01";
     gbXMLElement.append_attribute("SurfaceReferenceLocation") = "Centerline";
 
-    boost::optional<model::Facility> facility = model.getOptionalUniqueModelObject<model::Facility>();
-    if (facility) {
-      translateFacility(*facility, gbXMLElement);
-    }
+    // translateFacility is responsible to translate Surfaces, and calls translateBuilding, which is responsible to translate spaces
+    // so we do need to call it anyways.
+    translateFacility(model, gbXMLElement);
 
     // do constructions
     std::vector<model::ConstructionBase> constructionBases = model.getModelObjects<model::ConstructionBase>();
@@ -412,35 +411,36 @@ namespace gbxml {
     return true;
   }
 
-  boost::optional<pugi::xml_node> ForwardTranslator::translateFacility(const openstudio::model::Facility& facility, pugi::xml_node& parent)
+  boost::optional<pugi::xml_node> ForwardTranslator::translateFacility(const openstudio::model::Model& model, pugi::xml_node& parent)
   {
+
+    // `model` is `const`, so we shouldn't call getUniqueModelObject<model::Facility> which will **create** a new object in there.
+    boost::optional<model::Facility> _facility = model.getOptionalUniqueModelObject<model::Facility>();
+
     auto result = parent.append_child("Campus");
-    m_translatedObjects[facility.handle()] = result;
+    std::string name = "Facility";
 
-    boost::optional<std::string> name = facility.name();
+    if (_facility) {
+      m_translatedObjects[_facility->handle()] = result;
+      if (auto _s = _facility->name()) {
+        name = _s.get();
+      }
+    }
 
     // id
     result.append_attribute("id") = "Facility";
 
     // name
     auto nameElement = result.append_child("Name");
-    if (name) {
-      nameElement.text() = name.get().c_str();
-    } else {
-      nameElement.text() = "Facility";
-    }
-
-    model::Model model = facility.model();
+    nameElement.text() = name.c_str();
 
     // todo: translate location
 
-    // translate building
-    boost::optional<model::Building> building = model.getOptionalUniqueModelObject<model::Building>();
-    if (building) {
-      translateBuilding(*building, result);
-    }
+    // translate building: needs to be done even if not explicitly instantiated since that's what translates Spaces in particular.
+    translateBuilding(model, result);
 
     // translate surfaces
+    // TODO: JM 2020-06-18 Why is translateSpace not responsible to call this one?
     std::vector<model::Surface> surfaces = model.getConcreteModelObjects<model::Surface>();
     if (m_progressBar) {
       m_progressBar->setWindowTitle(toString("Translating Surfaces"));
@@ -477,44 +477,54 @@ namespace gbxml {
     return result;
   }
 
-  boost::optional<pugi::xml_node> ForwardTranslator::translateBuilding(const openstudio::model::Building& building, pugi::xml_node& parent)
+  boost::optional<pugi::xml_node> ForwardTranslator::translateBuilding(const openstudio::model::Model& model, pugi::xml_node& parent)
   {
+    // `model` is `const`, so we shouldn't call getUniqueModelObject<model::Building> which will **create** a new object in there.
+    // model::Building building = model.getUniqueModelObject<model::Building>();
+    boost::optional<model::Building> _building = model.getOptionalUniqueModelObject<model::Building>();
+
     auto result = parent.append_child("Building");
-    m_translatedObjects[building.handle()] = result;
+    std::string bName = "Building";
+    std::string bType = "Unknown";
+    if (_building) {
+      m_translatedObjects[_building->handle()] = result;
+      bName = _building->nameString();
+
+      if (boost::optional<std::string> _standardsBuildingType = _building->standardsBuildingType()) {
+        // TODO: map to gbXML types
+        // bType = escapeName(_standardsBuildingType.get()).c_str();
+      }
+
+      // space type
+      if (boost::optional<model::SpaceType> _spaceType = _building->spaceType()) {
+        //std::string spaceTypeName = _spaceType->nameString();
+        // TODO: map to gbXML types
+        // bType = escapeName(spaceTypeName).c_str();
+      }
+
+    }
 
     // id
-    std::string name = building.name().get();
-    result.append_attribute("id") = escapeName(name).c_str();
+    result.append_attribute("id") = escapeName(bName).c_str();
 
     // building type
-    //result.append_attribute("buildingType") = "Office";
-    result.append_attribute("buildingType") = "Unknown";
+    result.append_attribute("buildingType") = bType.c_str();
 
-    boost::optional<std::string> standardsBuildingType = building.standardsBuildingType();
-    if (standardsBuildingType) {
-      // todo: map to gbXML types
-      //result.append_attribute("buildingType") = escapeName(spaceTypeName).c_str();
-    }
+    if (_building) {
 
-    // space type
-    boost::optional<model::SpaceType> spaceType = building.spaceType();
-    if (spaceType) {
-      //std::string spaceTypeName = spaceType->name().get();
-      // todo: map to gbXML types
-      //result.append_attribute("buildingType", escapeName(spaceTypeName));
     }
 
     // name
     auto nameElement = result.append_child("Name");
-    nameElement.text() = name.c_str();
+    nameElement.text() = bName.c_str();
 
     // area
     auto areaElement = result.append_child("Area");
 
     // DLM: we want to use gbXML's definition of floor area which includes area from all spaces with people in them
     //double floorArea = building.floorArea();
 
-    std::vector<model::Space> spaces = building.spaces();
+    std::vector<model::Space> spaces = model.getConcreteModelObjects<model::Space>();
 
     double floorArea = 0;
     for (const model::Space& space : spaces) {
@@ -543,7 +553,7 @@ namespace gbxml {
     }
 
     // translate shading surface groups
-    model::ShadingSurfaceGroupVector shadingSurfaceGroups = building.model().getConcreteModelObjects<model::ShadingSurfaceGroup>();
+    model::ShadingSurfaceGroupVector shadingSurfaceGroups = model.getConcreteModelObjects<model::ShadingSurfaceGroup>();
     if (m_progressBar) {
       m_progressBar->setWindowTitle(toString("Translating Shading Surface Groups"));
       m_progressBar->setMinimum(0);
@@ -560,7 +570,7 @@ namespace gbxml {
     }
 
     // translate stories
-    model::BuildingStoryVector stories = building.model().getConcreteModelObjects<model::BuildingStory>();
+    model::BuildingStoryVector stories = model.getConcreteModelObjects<model::BuildingStory>();
     if (m_progressBar) {
       m_progressBar->setWindowTitle(toString("Translating Stories"));
       m_progressBar->setMinimum(0);

src/gbxml/ForwardTranslator.hpp

@@ -92,8 +92,12 @@ namespace gbxml {
 
     // listed in translation order
     bool translateModel(const openstudio::model::Model& model, pugi::xml_document& document);
-    boost::optional<pugi::xml_node> translateFacility(const openstudio::model::Facility& facility, pugi::xml_node& parent);
-    boost::optional<pugi::xml_node> translateBuilding(const openstudio::model::Building& building, pugi::xml_node& parent);
+
+    // Facility and Building could not be explicitly instantiated in the model, but the functions still need to be called so that Spaces and surfaces
+    // are translated. Facility and Building both are UniqueModelObjects, so passing model here as an argument is harmless
+    boost::optional<pugi::xml_node> translateFacility(const openstudio::model::Model& model, pugi::xml_node& parent);
+    boost::optional<pugi::xml_node> translateBuilding(const openstudio::model::Model& model, pugi::xml_node& parent);
+
     boost::optional<pugi::xml_node> translateSpace(const openstudio::model::Space& space, pugi::xml_node& parent);
     boost::optional<pugi::xml_node> translateShadingSurfaceGroup(const openstudio::model::ShadingSurfaceGroup& shadingSurfaceGroup, pugi::xml_node& parent);
     boost::optional<pugi::xml_node> translateBuildingStory(const openstudio::model::BuildingStory& story, pugi::xml_node& parent);

src/gbxml/MapEnvelope.cpp

@@ -182,9 +182,9 @@ namespace gbxml {
       double thickness = thicknessElement.text().as_double();
       double specificHeat = specificHeatElement.text().as_double();
 
-      std::string roughness = "MediumRough";
+      std::string roughness = "MediumRough"; // TODO: Shouldn't that be the same default as OS (Smooth)?
       if (!roughnessElement.empty()) {
-        roughness = roughnessElement.text().as_string();
+        roughness = roughnessElement.attribute("value").value();
       }
 
       openstudio::model::StandardOpaqueMaterial material(model);

src/gbxml/Test/ForwardTranslator_GTest.cpp

@@ -45,7 +45,9 @@
 #include "../../model/StandardOpaqueMaterial.hpp"
 #include "../../model/StandardOpaqueMaterial_Impl.hpp"
 #include "../../model/Space.hpp"
+#include "../../model/Space_Impl.hpp"
 #include "../../model/ThermalZone.hpp"
+#include "../../model/ThermalZone_Impl.hpp"
 
 #include "../../model/Model.hpp"
 
@@ -181,7 +183,7 @@ TEST_F(gbXMLFixture, ForwardTranslator_ConstructionLayers) {
 
   ASSERT_TRUE(model2);
   //std::cout << *model2 << std::endl;
-  auto osurf = model2->getModelObjectByName<Surface>(surfname);  
+  auto osurf = model2->getModelObjectByName<Surface>(surfname);
   ASSERT_TRUE(osurf);
   auto ocons = osurf->construction();
   ASSERT_TRUE(ocons);
@@ -193,3 +195,96 @@ TEST_F(gbXMLFixture, ForwardTranslator_ConstructionLayers) {
   EXPECT_TRUE(olayeredcons->layers()[2].optionalCast<MasslessOpaqueMaterial>());
   EXPECT_TRUE(olayeredcons->layers()[3].optionalCast<StandardOpaqueMaterial>());
 }
+
+TEST_F(gbXMLFixture, ForwardTranslator_NoFacility) {
+  // Test for #3314: gbXML translation does not roundtrip unless Facility object present
+
+  Model model;
+
+  Construction construction(model);
+  construction.setName("Construction1");
+
+  MaterialVector layers;
+
+  MasslessOpaqueMaterial material1(model);
+  material1.setName("Material1");
+  layers.push_back(material1);
+
+  StandardOpaqueMaterial material2(model);
+  material2.setName("Material2");
+  layers.push_back(material2);
+
+  MasslessOpaqueMaterial material3(model);
+  material3.setName("Material3");
+  layers.push_back(material3);
+
+  StandardOpaqueMaterial material4(model);
+  material4.setName("Material4");
+  material4.setRoughness("MediumSmooth");
+  layers.push_back(material4);
+
+  construction.setLayers(layers);
+
+  // Not instantiating facility nor building on purpose
+  // Facility facility = model.getUniqueModelObject<Facility>();
+  // Building building = model.getUniqueModelObject<Building>();
+
+  Space space(model);
+  space.setName("Space1");
+
+  Point3dVector points;
+  points.push_back(Point3d(0, 0, 1));
+  points.push_back(Point3d(0, 0, 0));
+  points.push_back(Point3d(1, 0, 0));
+  points.push_back(Point3d(1, 0, 1));
+
+  //std::string surfname("Surface 1"); // DLM: note this will fail because "Surface 1" gets round tripped as "Surface_1"
+  std::string surfname("Surface1");
+  Surface surface(points, model);
+  surface.setName(surfname);
+  surface.setConstruction(construction);
+  surface.setSpace(space);
+
+  ThermalZone zone(model);
+  zone.setName("Zone1");
+  space.setThermalZone(zone);
+
+  // save model for diffing
+  bool debug = false;
+  if (debug) {
+    path modelPath = resourcesPath() / openstudio::toPath("gbxml/ForwardTranslator_NoFacility_original.osm");
+    model.save(modelPath, true);
+  }
+
+  // Write out the XML
+  path p = resourcesPath() / openstudio::toPath("gbxml/ForwardTranslator_NoFacility.xml");
+
+  ForwardTranslator forwardTranslator;
+  bool test = forwardTranslator.modelToGbXML(model, p);
+
+  EXPECT_TRUE(test);
+
+  // Read the XML back in and check surface/space/zone were all translated
+  ReverseTranslator reverseTranslator;
+  boost::optional<Model> model2 = reverseTranslator.loadModel(p);
+
+  ASSERT_TRUE(model2);
+  //std::cout << *model2 << std::endl;
+  auto osurf = model2->getModelObjectByName<Surface>(surfname);
+  ASSERT_TRUE(osurf);
+  auto ospace = model2->getModelObjectByName<Space>(space.nameString());
+  ASSERT_TRUE(ospace);
+  auto ozone = model2->getModelObjectByName<ThermalZone>(zone.nameString()); // Dragostea Din Tei!
+  ASSERT_TRUE(ozone);
+
+  // This really tests a RT feature, but doesn't really matter. When diffing original & rountripped, I noticed a diff in Material:
+  // the roundtripped model has Roughness missing
+  auto omat = model2->getModelObjectByName<StandardOpaqueMaterial>("Material4");
+  ASSERT_TRUE(omat);
+  EXPECT_EQ("MediumSmooth", omat->roughness());
+
+  if (debug) {
+    path modelPath2 = resourcesPath() / openstudio::toPath("gbxml/ForwardTranslator_NoFacility_roundtripped.osm");
+    model2->save(modelPath2, true);
+  }
+}