-
Notifications
You must be signed in to change notification settings - Fork 14
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.
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
Environment_Engine: Compute Openings from a list of panels and a glazing ratio #3258
Environment_Engine: Compute Openings from a list of panels and a glazing ratio #3258
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus any compliance fixes the bot may find please @FelixMallinder-BH 😄
Additionally the PR title needs improving to meet our change log guidelines please
@BHoMBot check compliance |
@FraserGreenroyd to confirm, the following actions are now queued:
There are 1 requests in the queue ahead of you. |
Co-authored-by: Fraser Greenroyd <[email protected]>
Co-authored-by: Fraser Greenroyd <[email protected]>
Co-authored-by: Fraser Greenroyd <[email protected]>
Co-authored-by: Fraser Greenroyd <[email protected]>
Co-authored-by: Fraser Greenroyd <[email protected]>
Co-authored-by: Fraser Greenroyd <[email protected]>
Co-authored-by: Fraser Greenroyd <[email protected]>
@BHoMBot check for copyright-compliance |
@FelixMallinder-BH sorry, I didn't understand. |
@BHoMBot check copyright-compliance |
@FelixMallinder-BH to confirm, the following actions are now queued:
|
@FelixMallinder-BH fix requested for copyright headers. The errors with the copyright headers on the CS ( I will apply the fixes to every case detailed on the checks tab. If you want to perform the fixes in a different manner please resolve this manually and rerun the check. Each CS ( If you are happy for me to go ahead and perform this action, please reply with:
|
@BHoMBot fix copyright headers ref. |
@FelixMallinder-BH I'm sorry, but I cannot understand which check reference you are trying to provide. Please can you try again? |
@BHoMBot check compliance |
@BHoMBot check compliance |
@Tom-Kingstone to confirm, the following actions are now queued:
|
@Tom-Kingstone fix requested for copyright headers. The errors with the copyright headers on the CS ( I will apply the fixes to every case detailed on the checks tab. If you want to perform the fixes in a different manner please resolve this manually and rerun the check. Each CS ( If you are happy for me to go ahead and perform this action, please reply with:
|
@BHoMBot fix copyright headers ref. 20780849163 |
@Tom-Kingstone sorry, but the check you're asking me to make automatic fixes for has not failed, so I cannot make any changes at this point. |
@BHoMBot check copyright-compliance |
@Tom-Kingstone to confirm, the following actions are now queued:
|
@Tom-Kingstone to confirm, the following actions are now queued:
|
The check |
… moved private method logic to main public method
@Tom-Kingstone to confirm, the following actions are now queued:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested after changes using the test file provided, and the area returned is the same. Also the opening produced looks correct given the inputs, and ignoring panels is working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going by the test script everything works as intended. Good work @FelixMallinder-BH & @Tom-Kingstone! Happy to approve :)
Reviewed and re-tested @Tom-Kingstone 's changes, happy that they solved the previous issues and functionality follows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed the code and the tests done by the team and all looks good.
However, as noted in #3279, we do have issues with openings being created larger than their host panels but that comes from the Create
method rather than necessarily this method for now, so let's merge this PR for further testing, and then revisit this as necessary when handling #3279
@BHoMBot check required |
@FraserGreenroyd to confirm, the following actions are now queued:
|
The check |
The check |
@BHoMBot this is a DevOps instruction. I am requesting neutral checks on: unit-tests |
@BHoMBot check ready-to-merge |
@FraserGreenroyd to confirm, the following actions are now queued:
There are 19 requests in the queue ahead of you. |
@FraserGreenroyd I have provided neutral checks to the checks requested. These checks will need to be run properly to obtain full results. |
NOTE: Depends on
Issues addressed by this PR
Closes #3226
Added compute method, as described in the linked issue, the method calculates window sizes based on glazing ratio, for externally facing panels and creates openings.
Test files
OpeningsByGlazingByRatioTestScript.zip
Unzip, open in Rhino and check actual area is equal to expected area
Changelog
Additional comments