Add AirLoopHVAC to ThreeJS user data #4407

antoine-galataud · 2021-08-24T18:48:02Z

Pull request overview

First step in implementation of openstudiocoalition/OpenStudioApplication#413

Pull Request Author

Model API Changes / Additions
Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
Model API methods are tested (in src/model/test)
EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

tijcolem · 2021-08-25T22:56:21Z

Hi @antoine-galataud! Thank you for your contribution! This looks great. Before we can officially accept and merge would you mind reviewing our code contribution policy (https://www.openstudio.net/openstudio-contribution-policy) and then send me a consent email (my contact info is at the bottom). This is only a one time thing so any future contributions won't need this. =)

antoine-galataud · 2021-08-31T07:14:07Z

Builds failed but I can't access the CI tool (times out on every attempt). Is there an other convenient way to obtain build results?

tijcolem · 2021-08-31T22:59:46Z

@antoine-galataud I just emailed you concerning build logs.

This is the snippet of the error message for osx-incremental

/Users/jenkins/git/OpenStudioIncremental/develop/src/utilities/geometry/Test/ThreeJS_GTest.cpp:56:30: error: loop variable 'sceneChild' of type 'const openstudio::ThreeSceneChild' creates a copy from type 'const openstudio::ThreeSceneChild' [-Werror,-Wrange-loop-analysis]

  for (const ThreeSceneChild sceneChild : sceneChildren) {

                             ^

/Users/jenkins/git/OpenStudioIncremental/develop/src/utilities/geometry/Test/ThreeJS_GTest.cpp:56:8: note: use reference type 'const openstudio::ThreeSceneChild &' to prevent copying

  for (const ThreeSceneChild sceneChild : sceneChildren) {

       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

                             &

/Users/jenkins/git/OpenStudioIncremental/develop/src/utilities/geometry/Test/ThreeJS_GTest.cpp:62:39: error: loop variable 'metadata' of type 'const openstudio::ThreeModelObjectMetadata' creates a copy from type 'const openstudio::ThreeModelObjectMetadata' [-Werror,-Wrange-loop-analysis]

  for (const ThreeModelObjectMetadata metadata : scene->metadata().modelObjectMetadata()) {

                                      ^

/Users/jenkins/git/OpenStudioIncremental/develop/src/utilities/geometry/Test/ThreeJS_GTest.cpp:62:8: note: use reference type 'const openstudio::ThreeModelObjectMetadata &' to prevent copying

  for (const ThreeModelObjectMetadata metadata : scene->metadata().modelObjectMetadata()) {

       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

                                      &

2 errors generated.

make[2]: *** [src/utilities/CMakeFiles/openstudio_utilities_tests.dir/geometry/Test/ThreeJS_GTest.cpp.o] Error 1

make[2]: *** Waiting for unfinished jobs....

[ 23%] Building CXX object src/model/CMakeFiles/openstudio_model_tests.dir/test/ThreeJSReverseTranslator_GTest.cpp.o

make[1]: *** [src/utilities/CMakeFiles/openstudio_utilities_tests.dir/all] Error 2

make[1]: *** Waiting for unfinished jobs....

Consolidate compiler generated dependencies of target openstudio_radiance_tests

jmarrec · 2021-09-01T00:28:42Z

@tijcolem I already added review suggest for these unecessary copies, but I forgot to post my review.. so it was only visible on /files

Edit:: I apply these two sugestions

src/utilities/geometry/Test/ThreeJS_GTest.cpp

jmarrec · 2021-08-25T12:46:07Z

src/utilities/geometry/Test/ThreeJS_GTest.cpp

+  bool alhvac_metadata_found = false;
+  for (const ThreeModelObjectMetadata metadata : scene->metadata().modelObjectMetadata()) {
+    alhvac_metadata_found |= istringEqual("OS:AirLoopHVAC", metadata.iddObjectType()) && istringEqual("Air Loop HVAC 1", metadata.name());
+  }
+  ASSERT_TRUE(alhvac_metadata_found);


Suggested change

bool alhvac_metadata_found = false;

for (const ThreeModelObjectMetadata metadata : scene->metadata().modelObjectMetadata()) {

alhvac_metadata_found |= istringEqual("OS:AirLoopHVAC", metadata.iddObjectType()) && istringEqual("Air Loop HVAC 1", metadata.name());

}

ASSERT_TRUE(alhvac_metadata_found);

std::vector<ThreeModelObjectMetadata> metadatas = scene->metadata().modelObjectMetadata();

EXPECT_TRUE(std::any_of(metadatas.cbegin(), metadatas.cend(), [](const auto& metadata) {

return istringEqual("OS:AirLoopHVAC", metadata.iddObjectType()) && istringEqual("Air Loop HVAC 1", metadata.name();

})

);

nice construct, although makes it a bit harder to read IMHO. But my C++ skills are a bit rusted ;)

jmarrec · 2021-09-01T00:54:22Z

osx has a failing test but that's irrelevant here

[ RUN      ] BCLFixture.BCLComponent

/Users/jenkins/git/OpenStudioIncremental/develop/src/utilities/bcl/test/BCLComponent_GTest.cpp:44: Failure

Expected equality of these values:

  "philadelphia pa [724086 TMY2-13739]"

  component.name()

    Which is: ""

tijcolem · 2021-09-01T04:11:01Z

cool, yep that failing test is unrelated to this PR. Need to check on windows but Linux and Mac seems happy now.

antoine-galataud · 2021-09-01T06:48:17Z

Thanks @tijcolem and @jmarrec for reviewing and fixing.

…-galataud/OpenStudio into feature/threejs-airloop-userdata

tijcolem · 2021-09-03T13:48:42Z

src/utilities/geometry/Test/ThreeJS_GTest.cpp

+  // checking user data
+  std::vector<ThreeSceneChild> sceneChildren = scene->object().children();
+  for (const ThreeSceneChild& sceneChild : sceneChildren) {
+    EXPECT_DOUBLE_EQ(1, sceneChild.userData().airLoopHVACNames().size());


Can we change to EXPECT_DOUBLE_EQ(1, double(sceneChild.userData().airLoopHVACNames().size()));

windows is throwing the following due to treating warning as error when compiling.

D:\jenkins\openstudio\develop\src\utilities\geometry\Test\ThreeJS_GTest.cpp(57,5): error C2220: the following warning is treated as an error [D:\jenkins\openstudio\develop\build\src\utilities\openstudio_utilities_tests.vcxproj] D:\jenkins\openstudio\develop\src\utilities\geometry\Test\ThreeJS_GTest.cpp(57,5): warning C4244: 'argument': conversion from 'unsigned __int64' to 'RawType', possible loss of data [D:\jenkins\openstudio\develop\build\src\utilities\openstudio_utilities_tests.vcxproj]

Nah, you want EXPECT_EQ(1, xxx.size()); (or EXPECT_EQ(1u, xxx.size()); )

Even better!

src/utilities/geometry/Test/ThreeJS_GTest.cpp

ci-commercialbuildings · 2021-09-06T17:16:22Z

CI Results for fa11f06:

tijcolem

Thanks @antoine-galataud for making those changes. I looked CI and Ubuntu 20.04 and osx are unrelated so this looks good to drop in!

antoine-galataud added 5 commits August 24, 2021 15:17

add air loop HVAC info in ThreeJS user data

9a0cb53

fix unit test syntax

1eb9327

improve test coverage

05d2194

fix test data: air loop HVAC user data is of array type

3a36e3f

remove assertion

d54020d

tijcolem added component - Utilities Geometry Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Aug 30, 2021

Merge branch 'develop' into feature/threejs-airloop-userdata

1e3dd99

jmarrec reviewed Sep 1, 2021

View reviewed changes

Apply suggestions from code review

2660be2

antoine-galataud added 2 commits September 1, 2021 09:31

apply suggested test code change

bd8f372

Merge branch 'feature/threejs-airloop-userdata' of github.com:antoine…

e8c5143

…-galataud/OpenStudio into feature/threejs-airloop-userdata

tijcolem reviewed Sep 3, 2021

View reviewed changes

fix warning on windows build

f3dd7c1

antoine-galataud requested a review from tijcolem September 4, 2021 14:21

jmarrec requested changes Sep 6, 2021

View reviewed changes

src/utilities/geometry/Test/ThreeJS_GTest.cpp Outdated Show resolved Hide resolved

Update src/utilities/geometry/Test/ThreeJS_GTest.cpp

fa11f06

jmarrec approved these changes Sep 6, 2021

View reviewed changes

tijcolem approved these changes Sep 7, 2021

View reviewed changes

tijcolem merged commit b29bc47 into NREL:develop Sep 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AirLoopHVAC to ThreeJS user data #4407

Add AirLoopHVAC to ThreeJS user data #4407

antoine-galataud commented Aug 24, 2021

tijcolem commented Aug 25, 2021

antoine-galataud commented Aug 31, 2021

tijcolem commented Aug 31, 2021

jmarrec commented Sep 1, 2021 •

edited

Loading

jmarrec Aug 25, 2021 •

edited

Loading

antoine-galataud Sep 1, 2021

jmarrec commented Sep 1, 2021

tijcolem commented Sep 1, 2021

antoine-galataud commented Sep 1, 2021

tijcolem Sep 3, 2021

jmarrec Sep 6, 2021

tijcolem Sep 7, 2021

ci-commercialbuildings commented Sep 6, 2021 •

edited

Loading

tijcolem left a comment

Add AirLoopHVAC to ThreeJS user data #4407

Add AirLoopHVAC to ThreeJS user data #4407

Conversation

antoine-galataud commented Aug 24, 2021

Pull request overview

Pull Request Author

Review Checklist

tijcolem commented Aug 25, 2021

antoine-galataud commented Aug 31, 2021

tijcolem commented Aug 31, 2021

jmarrec commented Sep 1, 2021 • edited Loading

jmarrec Aug 25, 2021 • edited Loading

Choose a reason for hiding this comment

antoine-galataud Sep 1, 2021

Choose a reason for hiding this comment

jmarrec commented Sep 1, 2021

tijcolem commented Sep 1, 2021

antoine-galataud commented Sep 1, 2021

tijcolem Sep 3, 2021

Choose a reason for hiding this comment

jmarrec Sep 6, 2021

Choose a reason for hiding this comment

tijcolem Sep 7, 2021

Choose a reason for hiding this comment

ci-commercialbuildings commented Sep 6, 2021 • edited Loading

tijcolem left a comment

Choose a reason for hiding this comment

jmarrec commented Sep 1, 2021 •

edited

Loading

jmarrec Aug 25, 2021 •

edited

Loading

ci-commercialbuildings commented Sep 6, 2021 •

edited

Loading