Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue 4361 #4424

Merged
merged 19 commits into from Nov 19, 2021
Merged

Issue 4361 #4424

merged 19 commits into from Nov 19, 2021

Conversation

ggartside
Copy link
Collaborator

@ggartside ggartside commented Aug 30, 2021 •
edited by jmarrec

Pull request overview

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

@ggartside
Added SubSurface::totalArea to get the area of a sub surface that inc…
a473f66 
…ludes the area of the frame. Modified PlanarSurface::netArea to call the SubSurface::totalArea. Added a unit test Issue_4361 to SubSurface_Gtest
@ggartside
Modified windowToWallRatio to call SubSurface totalArea. Modified tes…
93891ef 
…t to include window wall ratio check
@ggartside
Renamed totalArea to roughOpeningArea
1e54ead 
Added method totalAreaOfSubSurfaces to Surface
Added polygonInPolygon methof to intersection
macumber
macumber reviewed Aug 31, 2021
View reviewed changes
src/model/SubSurface.hpp Outdated Show resolved Hide resolved
macumber
macumber reviewed Aug 31, 2021
View reviewed changes
src/model/SubSurface.hpp Outdated Show resolved Hide resolved
macumber
macumber reviewed Aug 31, 2021
View reviewed changes
src/model/SubSurface.hpp Outdated Show resolved Hide resolved
@tijcolem 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 Sep 2, 2021
ggartside and others added 7 commits September 15, 2021 12:03
@ggartside @macumber
Update src/model/SubSurface.hpp
2faa5b3 
Co-authored-by: Dan Macumber <dan.macumber@gmail.com>
@ggartside @macumber
Update src/model/SubSurface.hpp
0cf9f62 
Co-authored-by: Dan Macumber <dan.macumber@gmail.com>
@ggartside
Includes sub surface multiplier when caclulating the rough opening ar…
5d3f7d8 
…ea for the window wall ratio calculartion
@ggartside
Merge branch 'issue-4361' of https://github.com/NREL/OpenStudio into …
6c6c709 
…issue-4361
@ggartside
I took the code change suggestions but didnt realise if the method na…
1e5bfa4 
…me was chanegd in the header utomatically it wouldnt change all references
@ggartside
Fix building issue on macOS, linux
da8da67
@tijcolem
Merge branch 'develop' into issue-4361
3d95397
@tijcolem
Copy link
Collaborator

tijcolem commented Oct 8, 2021

@ggartside Can you remove those .patch files? I don't think you want to include these @macumber mentioned.

@ggartside
Copy link
Collaborator Author

ggartside commented Oct 8, 2021 •
edited

I think you already removed the patches?

@tijcolem
Copy link
Collaborator

tijcolem commented Oct 8, 2021

@ggartside Yep, I just pushed up a commit to remove those.

jmarrec
jmarrec requested changes Oct 21, 2021
View reviewed changes
Copy link
Collaborator

@jmarrec jmarrec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ggartside I left some review comments. Most of it is just syntax/nitpicking, but I did leave three marked * [ ] TODO: that need attention (some are questions, and I would be satisfied if the answer is just "No, it is intended")

src/energyplus/Test/SubSurface_GTest.cpp Outdated Show resolved Hide resolved
src/energyplus/Test/SubSurface_GTest.cpp Outdated Show resolved Hide resolved
src/energyplus/Test/SubSurface_GTest.cpp Outdated Show resolved Hide resolved
src/energyplus/Test/SubSurface_GTest.cpp Outdated Show resolved Hide resolved
src/energyplus/Test/SubSurface_GTest.cpp Outdated Show resolved Hide resolved
src/model/Surface.cpp Outdated
std::reverse(vertices.begin(), vertices.end());
}
roughOpenings[subSurface] = vertices;
break;
Copy link
Collaborator

@jmarrec jmarrec Oct 21, 2021 •
edited by ggartside

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • TODO: Is the break intended? I don't understand everything that's going on, but don't you want to fix all of them?
    Also, why not make this check directly in the loop above when populating roughOpenins?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are correct an original iteration had a loop that checked each vertex and the break was to hop out of that loop - good find! I also found I'd re-used the name vertices in the second loop and had missed a place where I needed to get the inverse of the transform

I probably could have combined the first two loops, but I think separating them makes the code easier to follow

  1. First loop sets up the sub surface rough opening vertices in the coordinate space of the parent surface
  2. Second loop checks these vertices to see if they extend outside the parent surface
  3. Third loop checks each subsurface for overlaps with adjacent sub surfaces

src/model/Surface.cpp Outdated Show resolved Hide resolved
src/model/Surface.cpp Outdated Show resolved Hide resolved
src/model/Surface.cpp Outdated Show resolved Hide resolved
src/utilities/geometry/Intersection.cpp Outdated Show resolved Hide resolved
jmarrec
jmarrec reviewed Oct 21, 2021
View reviewed changes
Copy link
Collaborator

@jmarrec jmarrec left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applied my nitpicking changes and moved the tests to ModelFixture. I resolved all outdated review comments as a result. You have 4 reviews TODO left in the review above.

src/energyplus/Test/SubSurface_GTest.cpp Outdated Show resolved Hide resolved
@tijcolem
Copy link
Collaborator

@ggartside Just following up on this PR. Have you addressed the TODO @jmarrec requested? Let's do another review and then we can hopefully get this merged in.

@ci-commercialbuildings
Copy link
Collaborator

ci-commercialbuildings commented Nov 16, 2021 •
edited

CI Results for 155a734:

@tijcolem
Copy link
Collaborator

Looks like the review comments have been addressed on this so I'm going to merge.

@tijcolem tijcolem merged commit 6916c7e into develop Nov 19, 2021
@tijcolem tijcolem deleted the issue-4361 branch November 19, 2021 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@macumber macumber macumber left review comments

@jmarrec jmarrec jmarrec requested changes

Assignees
No one assigned
Labels
component - Utilities Geometry Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OpenStudio API-reported surface "netArea" (when dealing with Frame & Divider objects)
5 participants
@ggartside @tijcolem @ci-commercialbuildings @macumber @jmarrec