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

Add Spandrel Force Result #457

Merged
merged 14 commits into from
Oct 31, 2024
Merged

Add Spandrel Force Result #457

merged 14 commits into from
Oct 31, 2024

Conversation

GCRA101
Copy link
Contributor

@GCRA101 GCRA101 commented Aug 12, 2024 •
edited
Loading

Issues addressed by this PR

Closes #430

Test files

Grasshopper File
https://burohappold.sharepoint.com/:u:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/ETABS_Toolkit/%23457-AddSpandrelForceResult/Test%20Script.gh?csf=1&web=1&e=caphlX
ETABS File
https://burohappold.sharepoint.com/:u:/r/sites/BHoM/02_Current/12_Scripts/02_Pull%20Request/BHoM/ETABS_Toolkit/%23457-AddSpandrelForceResult/Test%20ETABS%20Model.EDB?csf=1&web=1&e=6N8BDm

TestScreenshot

Changelog

  • Added SpandrelForce Class to the ETABS_oM Results namespace
  • Added SpandrelForce value to Enum PierAndSpandrelResultType in the Requests namespace
  • Added calls to ETABS API functions to extract Spandrel Forces from ETABS Model in the ETABS Adapter

@GCRA101 GCRA101 self-assigned this Aug 12, 2024
@GCRA101 GCRA101 added the type:feature New capability or enhancement label Aug 12, 2024
@GCRA101 GCRA101 added this to the BHoM 7.3 β MVP milestone Aug 12, 2024
IsakNaslundBh
IsakNaslundBh previously requested changes Aug 16, 2024
View reviewed changes
Copy link
Contributor

@IsakNaslundBh IsakNaslundBh left a comment

Choose a reason for hiding this comment

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

Have not tested, but just as a first comment, please have a look at the changes made by @peterjamesnugent in #455. Think the spandrel forces should follow suit, and also not inherit from BarForce

@Chrisshort92
Copy link
Contributor

I have tested the functionality and happy that it works as expected with the test scripts provided. @GCRA101 I will wait until the comments above have been addressed before approving this PR.

@GCRA101
Change inheritance of SpandrelForce from BarForce to BarResult
1511ecd 
In line with what already done with PierForce
@GCRA101
Copy link
Contributor Author

GCRA101 commented Sep 2, 2024

@IsakNaslundBh , @Chrisshort92 ,
thank you for reviewing and for your comments.
I've updated the inheritance of the SpandrelForce Class to be from BarResult rather than from BarForce.

Updated code pushed to origin for your final check.

SpandrelForceClassInheritance

peterjamesnugent
peterjamesnugent previously requested changes Sep 3, 2024
View reviewed changes
Copy link
Member

@peterjamesnugent peterjamesnugent left a comment

Choose a reason for hiding this comment

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

I couldn't get it to build without changes to the SpandrelForce, also it needs to update from origin/develop.

ETABS_oM/Results/SpandrelForce.cs Outdated Show resolved Hide resolved
ETABS_oM/Results/SpandrelForce.cs Show resolved Hide resolved
ETABS_oM/Results/SpandrelForce.cs Show resolved Hide resolved
ETABS_oM/Results/SpandrelForce.cs Outdated Show resolved Hide resolved
ETABS_oM/Results/SpandrelForce.cs Show resolved Hide resolved
Etabs_Adapter/CRUD/Read/Results/PierAndSpandrelResults.cs Show resolved Hide resolved
ETABS_oM/Requests/PierAndSpandrelForceRequest.cs Outdated Show resolved Hide resolved
ETABS_oM/Requests/Enum/PierAndSpandrelResultType.cs Show resolved Hide resolved
GCRA101 and others added 5 commits September 4, 2024 09:49
@GCRA101 @peterjamesnugent
Update Inheritance header for SpandrelForce Class
5ef3bbd 
Co-authored-by: Peter Nugent <Peter.Nugent@burohappold.com>
@GCRA101 @peterjamesnugent
Add attributes to SpandrelForce Class
f9d3cab 
Co-authored-by: Peter Nugent <Peter.Nugent@burohappold.com>
@GCRA101 @peterjamesnugent
Update attributes assignment within Constructor
15c6898 
Co-authored-by: Peter Nugent <Peter.Nugent@burohappold.com>
@GCRA101
Merge branch 'develop' of https://github.com/BHoM/ETABS_Toolkit into …
9537538 
…ETABS_Toolkit-#430-AddSpandrelForceResult
@GCRA101
Add missing namespace references to SpandrelForce class
b09f44e
@GCRA101
Copy link
Contributor Author

GCRA101 commented Sep 4, 2024

@peterjamesnugent thanks for your comments.
I've updated the code in line with them. Retested using the linked etabs model file and the gh script. All works fine.

GCRA101
GCRA101 commented Sep 4, 2024
View reviewed changes
Copy link
Contributor Author

@GCRA101 GCRA101 left a comment

Choose a reason for hiding this comment

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

Thank for the comments.
I've incorporated them in the code and tested it in Grasshopper.
Feature works as expected.

Chrisshort92
Chrisshort92 reviewed Sep 4, 2024
View reviewed changes
Copy link
Contributor

@Chrisshort92 Chrisshort92 left a comment

Choose a reason for hiding this comment

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

Functionality has been tested and approved against the latest changes based on previous conversations about with @IsakNaslundBh and @peterjamesnugent.

@Chrisshort92 Chrisshort92 dismissed stale reviews from IsakNaslundBh and peterjamesnugent September 4, 2024 11:26

Changes have been adopted in commits above.

Chrisshort92
Chrisshort92 previously approved these changes Sep 4, 2024
View reviewed changes
Copy link
Contributor

@Chrisshort92 Chrisshort92 left a comment

Choose a reason for hiding this comment

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

Functionality checked and code changes adopted. Approved for merge

@Chrisshort92
Copy link
Contributor

@BHoMBot check required

@bhombot-ci BHoMBot-CI
Copy link

bhombot-ci bot commented Sep 4, 2024

@Chrisshort92 to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer

@GCRA101
Copy link
Contributor Author

GCRA101 commented Sep 4, 2024

@BHoMBot check copyright-compliance

@bhombot-ci BHoMBot-CI
Copy link

bhombot-ci bot commented Sep 4, 2024

@GCRA101 to confirm, the following actions are now queued:

  • check copyright-compliance

@GCRA101
Copy link
Contributor Author

GCRA101 commented Sep 4, 2024

@BHoMBot check ready-to-merge

@bhombot-ci BHoMBot-CI
Copy link

bhombot-ci bot commented Sep 4, 2024

@GCRA101 to confirm, the following actions are now queued:

  • check ready-to-merge

peterjamesnugent
peterjamesnugent reviewed Sep 4, 2024
View reviewed changes
Copy link
Member

@peterjamesnugent peterjamesnugent left a comment

Choose a reason for hiding this comment

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

A few more changes need to be made to align it with the other ReadResults. Happy to run it through on a call tomorrow.

ETABS_oM/Results/SpandrelForce.cs Outdated Show resolved Hide resolved
ETABS_oM/Requests/PierAndSpandrelForceRequest.cs Outdated Show resolved Hide resolved
@Chrisshort92
Update SpandrelForce.cs
bdeac8c 
Duplication removed.
@peterjamesnugent peterjamesnugent added the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Sep 6, 2024
@peterjamesnugent peterjamesnugent removed this from the BHoM 7.3 β MVP milestone Sep 9, 2024
@peterjamesnugent peterjamesnugent removed the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Oct 1, 2024
@peterjamesnugent peterjamesnugent added this to the BHoM 7.4 β MVP milestone Oct 1, 2024
@GCRA101
Copy link
Contributor Author

GCRA101 commented Oct 1, 2024

@peterjamesnugent , @Chrisshort92

I've updated the initialization call of the PierAndSpandrel enum as suggested by Isak in today's call.
You can find the updated code for your review in the latest commit.
c01050f

peterjamesnugent
peterjamesnugent previously requested changes Oct 3, 2024
View reviewed changes
Copy link
Member

@peterjamesnugent peterjamesnugent left a comment

Choose a reason for hiding this comment

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

Some changes from csproj need to be removed.

Otherwise I am happy with the code changes - just @Chrisshort92 to review functionality.

Etabs_Adapter/Etabs_Adapter.csproj Outdated Show resolved Hide resolved
Etabs_Adapter/Etabs_Adapter.csproj Outdated Show resolved Hide resolved
Chrisshort92
Chrisshort92 approved these changes Oct 14, 2024
View reviewed changes
Copy link
Contributor

@Chrisshort92 Chrisshort92 left a comment

Choose a reason for hiding this comment

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

Functionality tested with most recent changes. Component working as expected.

@peterjamesnugent
Copy link
Member

@BHoMBot check required

@bhombot-ci BHoMBot-CI
Copy link

bhombot-ci bot commented Oct 23, 2024

@peterjamesnugent to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer

@peterjamesnugent peterjamesnugent dismissed their stale review October 23, 2024 14:17

review now outdated

@GCRA101
Copy link
Contributor Author

GCRA101 commented Oct 28, 2024

@BHoMBot check copyright-compliance

@bhombot-ci BHoMBot-CI
Copy link

bhombot-ci bot commented Oct 28, 2024

@GCRA101 to confirm, the following actions are now queued:

  • check copyright-compliance

@GCRA101
Copy link
Contributor Author

GCRA101 commented Oct 28, 2024

@BHoMBot check null-handling

@bhombot-ci BHoMBot-CI
Copy link

bhombot-ci bot commented Oct 28, 2024

@GCRA101 to confirm, the following actions are now queued:

  • check null-handling

@GCRA101
Copy link
Contributor Author

GCRA101 commented Oct 28, 2024

@BHoMBot check ready-to-merge

@bhombot-ci BHoMBot-CI
Copy link

bhombot-ci bot commented Oct 28, 2024

@GCRA101 to confirm, the following actions are now queued:

  • check ready-to-merge

@peterjamesnugent peterjamesnugent merged commit edc11e0 into develop Oct 31, 2024
10 checks passed
@peterjamesnugent peterjamesnugent deleted the ETABS_Toolkit-#430-AddSpandrelForceResult branch October 31, 2024 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Chrisshort92 Chrisshort92 Chrisshort92 approved these changes

@IsakNaslundBh IsakNaslundBh IsakNaslundBh left review comments

@peterjamesnugent peterjamesnugent peterjamesnugent left review comments

Assignees

@GCRA101 GCRA101

Labels
type:feature New capability or enhancement
Projects
None yet
Milestone
BHoM 7.4 β MVP
Development

Successfully merging this pull request may close these issues.

Add SpandrelForce to PierandSpandrelForceResultRequest Option
4 participants
@GCRA101 @Chrisshort92 @peterjamesnugent @IsakNaslundBh