From 92c3c819fa777b8b99391bb9aa5c57884e0f0091 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Thu, 7 May 2020 09:30:27 +0200 Subject: [PATCH 1/3] Add test for #3943 --- CMakeLists.txt | 2 +- .../test/ThreeJSForwardTranslator_GTest.cpp | 28 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 66a0ea89972..bc6023668cc 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -6,7 +6,7 @@ set(CMAKE_CXX_STANDARD_REQUIRED ON) # Do not enable compiler specific extensions, for eg on GCC use -std=c++1z (=c++17) and not -std=gnu++17 set(CMAKE_CXX_EXTENSIONS OFF) -# Use ccache is available, has to be before "project()" +# Use ccache if available, has to be before "project()" find_program(CCACHE_PROGRAM ccache) if(CCACHE_PROGRAM) # Support Unix Makefiles and Ninja diff --git a/src/model/test/ThreeJSForwardTranslator_GTest.cpp b/src/model/test/ThreeJSForwardTranslator_GTest.cpp index 39a91fadf6e..20a06e26785 100644 --- a/src/model/test/ThreeJSForwardTranslator_GTest.cpp +++ b/src/model/test/ThreeJSForwardTranslator_GTest.cpp @@ -38,6 +38,8 @@ #include "../Space_Impl.hpp" #include "../Surface.hpp" #include "../Surface_Impl.hpp" +#include "../ConstructionAirBoundary.hpp" +#include "../ConstructionAirBoundary_Impl.hpp" #include "../../utilities/geometry/ThreeJS.hpp" @@ -83,3 +85,29 @@ TEST_F(ModelFixture,ThreeJSForwardTranslator_ExampleModel) { EXPECT_EQ(model.getConcreteModelObjects().size(), model2->getConcreteModelObjects().size()); EXPECT_EQ(model.getConcreteModelObjects().size(), model2->getConcreteModelObjects().size()); } + +TEST_F(ModelFixture,ThreeJSForwardTranslator_ConstructionAirBoundary) { + + ThreeJSForwardTranslator ft; + ThreeJSReverseTranslator rt; + openstudio::path out; + + Model m = exampleModel(); + EXPECT_FALSE(m.getConcreteModelObjects().empty()); + + ThreeScene scene = ft.modelToThreeJS(m, false); + + // Ensure we get no errors or warnings, generally speaking. + // Here I'm especially after #3943: "Unknown iddObjectType 'OS:Construction:AirBoundary'" + EXPECT_EQ(0, ft.errors().size()); + EXPECT_EQ(0, ft.warnings().size()); + + for (const auto& error : ft.errors()){ + EXPECT_TRUE(false) << "Error: " << error.logMessage(); + } + + for (const auto& warning : ft.warnings()){ + EXPECT_TRUE(false) << "Warning: " << warning.logMessage(); + } + +} From 434a9dadf3aa7561bad0ef5cd503f078f17343ba Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Thu, 7 May 2020 11:39:35 +0200 Subject: [PATCH 2/3] #3943 update test --- .../test/ThreeJSForwardTranslator_GTest.cpp | 55 +++++++++++++++++-- 1 file changed, 50 insertions(+), 5 deletions(-) diff --git a/src/model/test/ThreeJSForwardTranslator_GTest.cpp b/src/model/test/ThreeJSForwardTranslator_GTest.cpp index 20a06e26785..1f3f8cb042e 100644 --- a/src/model/test/ThreeJSForwardTranslator_GTest.cpp +++ b/src/model/test/ThreeJSForwardTranslator_GTest.cpp @@ -39,10 +39,12 @@ #include "../Surface.hpp" #include "../Surface_Impl.hpp" #include "../ConstructionAirBoundary.hpp" -#include "../ConstructionAirBoundary_Impl.hpp" +#include "../Construction.hpp" #include "../../utilities/geometry/ThreeJS.hpp" +#include + using namespace openstudio; using namespace openstudio::model; @@ -57,6 +59,18 @@ TEST_F(ModelFixture,ThreeJSForwardTranslator_ExampleModel) { // triangulated, for display ThreeScene scene = ft.modelToThreeJS(model, true); + // Ensure we get no errors or warnings, generally speaking. + EXPECT_EQ(0, ft.errors().size()); + EXPECT_EQ(0, ft.warnings().size()); + + for (const auto& error : ft.errors()){ + EXPECT_TRUE(false) << "Error: " << error.logMessage(); + } + + for (const auto& warning : ft.warnings()){ + EXPECT_TRUE(false) << "Warning: " << warning.logMessage(); + } + std::string json = scene.toJSON(); EXPECT_TRUE(ThreeScene::load(json)); @@ -68,6 +82,19 @@ TEST_F(ModelFixture,ThreeJSForwardTranslator_ExampleModel) { // not triangulated, for model transport/translation scene = ft.modelToThreeJS(model, false); + + // Ensure we get no errors or warnings, generally speaking. + EXPECT_EQ(0, ft.errors().size()); + EXPECT_EQ(0, ft.warnings().size()); + + for (const auto& error : ft.errors()){ + EXPECT_TRUE(false) << "Error: " << error.logMessage(); + } + + for (const auto& warning : ft.warnings()){ + EXPECT_TRUE(false) << "Warning: " << warning.logMessage(); + } + json = scene.toJSON(); EXPECT_TRUE(ThreeScene::load(json)); @@ -89,11 +116,13 @@ TEST_F(ModelFixture,ThreeJSForwardTranslator_ExampleModel) { TEST_F(ModelFixture,ThreeJSForwardTranslator_ConstructionAirBoundary) { ThreeJSForwardTranslator ft; - ThreeJSReverseTranslator rt; - openstudio::path out; - Model m = exampleModel(); - EXPECT_FALSE(m.getConcreteModelObjects().empty()); + Model m; + ConstructionAirBoundary cAirBoundary(m); + cAirBoundary.setName("Construction_Air_Boundary"); + + Construction c(m); + c.setName("RegularConstruction"); ThreeScene scene = ft.modelToThreeJS(m, false); @@ -110,4 +139,20 @@ TEST_F(ModelFixture,ThreeJSForwardTranslator_ConstructionAirBoundary) { EXPECT_TRUE(false) << "Warning: " << warning.logMessage(); } + + // Materials are named like "prefix_" + + auto checkIfMaterialExist = [](const auto& materials, const std::string& containedString) { + auto it = std::find_if(materials.cbegin(), materials.cend(), + [&containedString](const auto& mat){ + return mat.name().find(containedString) != std::string::npos; + } + ); + return it != materials.cend(); + }; + + auto materials = scene.materials(); + EXPECT_TRUE(checkIfMaterialExist(materials, "RegularConstruction")); + EXPECT_FALSE(checkIfMaterialExist(materials, "Construction_Air_Boundary")); // Instead it should have been skipped to be replace by "AirWall" + EXPECT_TRUE(checkIfMaterialExist(materials, "AirWall")); + } From de3a365ef6ea9673a8677740f8cbcf9a25936706 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Thu, 7 May 2020 11:41:35 +0200 Subject: [PATCH 3/3] Proposed fix for #3943: assign "AirWall" material to all ConstructionAirBoundary --- src/model/ThreeJSForwardTranslator.cpp | 25 ++++++++++++++++++------- src/utilities/geometry/ThreeJS.cpp | 16 ++++++++++------ 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/src/model/ThreeJSForwardTranslator.cpp b/src/model/ThreeJSForwardTranslator.cpp index 07e4251a48a..dda2c1d06a2 100644 --- a/src/model/ThreeJSForwardTranslator.cpp +++ b/src/model/ThreeJSForwardTranslator.cpp @@ -32,6 +32,8 @@ #include "RenderingColor.hpp" #include "ConstructionBase.hpp" #include "ConstructionBase_Impl.hpp" +#include "ConstructionAirBoundary.hpp" +#include "ConstructionAirBoundary_Impl.hpp" #include "ThermalZone.hpp" #include "ThermalZone_Impl.hpp" #include "SpaceType.hpp" @@ -85,15 +87,19 @@ namespace openstudio { // make construction materials for (auto& construction : model.getModelObjects()){ - boost::optional color = construction.renderingColor(); - if (!color){ - color = RenderingColor(model); - construction.setRenderingColor(*color); + // If it's ConstructionAirBoundary, we'll later use the standard material "AirWall" + if (!construction.optionalCast()) { + boost::optional color = construction.renderingColor(); + if (!color){ + color = RenderingColor(model); + construction.setRenderingColor(*color); + } + std::string name = getObjectThreeMaterialName(construction); + addThreeMaterial(materials, materialMap, makeThreeMaterial(name, toThreeColor(color->renderingRedValue(), color->renderingBlueValue(), color->renderingGreenValue()), 1, ThreeSide::DoubleSide)); } - std::string name = getObjectThreeMaterialName(construction); - addThreeMaterial(materials, materialMap, makeThreeMaterial(name, toThreeColor(color->renderingRedValue(), color->renderingBlueValue(), color->renderingGreenValue()), 1, ThreeSide::DoubleSide)); } + // make thermal zone materials for (auto& thermalZone : model.getConcreteModelObjects()){ boost::optional color = thermalZone.renderingColor(); @@ -259,7 +265,12 @@ namespace openstudio if (construction) { userData.setConstructionName(construction->nameString()); - userData.setConstructionMaterialName(getObjectThreeMaterialName(*construction)); + // If this is a ConstructionAirBoundary, then set to the standard material "AirWall" + if (construction->optionalCast()) { + userData.setConstructionMaterialName("AirWall"); + } else { + userData.setConstructionMaterialName(getObjectThreeMaterialName(*construction)); + } } if (space) diff --git a/src/utilities/geometry/ThreeJS.cpp b/src/utilities/geometry/ThreeJS.cpp index 9234f47f7b1..5f694fb3dde 100644 --- a/src/utilities/geometry/ThreeJS.cpp +++ b/src/utilities/geometry/ThreeJS.cpp @@ -77,16 +77,20 @@ namespace openstudio{ std::string result; if (istringEqual(iddObjectType, "OS:Construction")){ result = "Construction_" + name; - }else if (istringEqual(iddObjectType, "OS:ThermalZone")){ + } else if (istringEqual(iddObjectType, "OS:ThermalZone")){ result = "ThermalZone_" + name; - }else if (istringEqual(iddObjectType, "OS:SpaceType")){ + } else if (istringEqual(iddObjectType, "OS:SpaceType")){ result = "SpaceType_" + name; - }else if (istringEqual(iddObjectType, "OS:BuildingStory")){ + } else if (istringEqual(iddObjectType, "OS:BuildingStory")){ result = "BuildingStory_" + name; - }else if (istringEqual(iddObjectType, "OS:BuildingUnit")){ + } else if (istringEqual(iddObjectType, "OS:BuildingUnit")){ result = "BuildingUnit_" + name; - } else{ - LOG_FREE(Error, "getObjectMaterialName", "Unknown iddObjectType '" << iddObjectType << "'"); + } else if (istringEqual(iddObjectType, "OS:Construction:AirBoundary")){ + // This shouldn't happen + LOG_FREE(Error, "getObjectThreeMaterialName", "Didn't expect it would be called for '" << iddObjectType << "' (name = '" << name << "')"); + result = "ConstructionAirBoundary_" + name; + } else { + LOG_FREE(Error, "getObjectThreeMaterialName", "Unknown iddObjectType '" << iddObjectType << "'"); } return result; }