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.

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

Fix read BarUniformlyDistributedLoad #188

Merged
merged 3 commits into from
Aug 23, 2019

Conversation

JosefTaylor
Copy link
Contributor

@JosefTaylor JosefTaylor commented Jul 11, 2019

The fake bar is because I can't access ReadBar from this namespace.

Issues addressed by this PR

Closes #186
Closes #187
Closes #172
Closes #173

Test files

https://burohappold.sharepoint.com/:f:/r/sites/BHoM/02_Current/12_Scripts/01_Test%20Scripts/ETABS_Toolkit/Etabs_Toolkit-Issue187-PullBarDistributedLoad?csf=1&e=NrMJUA

Changelog

  • Corrected problem with bar uniform loads where loads would be read with nonsense parameters.
  • Added an empty element to each load containing the ETABS_id so that loads can be correlated to existing elements.
  • Added a note to let the user know the element is empty but contains the correct id.

Additional comments

…ct ID.

The fake bar is because I can't access ReadBar from this namespace.
@JosefTaylor JosefTaylor added the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Jul 11, 2019
@peterjamesnugent
Copy link
Member

@JosefTaylor Add a warning message to explain the bar is fake and reference the Wiki for a more detailed explanation as discussed in weekly structures call.

@FraserGreenroyd
Copy link
Contributor

@JosefTaylor can you nominate some reviewers for this please so this can be reviewed and closed out 😄 maybe @enarhi or @IsakNaslundBh ? Cheers!

@JosefTaylor JosefTaylor removed the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Jul 26, 2019
Josef Taylor added 2 commits July 26, 2019 11:06
Also had to change the areaUniformLoad section to be similar to barsUniformLoad
@IsakNaslundBh
Copy link
Contributor

I am happy to review this.

Will have a look at it next week.

@enarhi
Copy link
Member

enarhi commented Aug 2, 2019

I can review early next week as well. Been a bit swamped this week.

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.

This currently does not pull anything at all from one of my project models.

@JosefTaylor Do you have any testfiles I could try this on to rule out that there is not a problem with my specific model?

Also, think there are quite a lot of things in here that would need quite a big refactoring/general change. For example, this whole section should be moved into the adapter read, then it would not be a problem getting out the real bar. If you have time this might be worth doing now, as this would avoid the need for the fake bars.

Also, now the code tries to real all loads of all types, then filter afterwards. Think this should be refactored into separate methods instead. (again not really related to the changes you have made). Issue for this opened here #190

Copy link
Member

@enarhi enarhi left a comment

Choose a reason for hiding this comment

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

Approved for functionality, refactoring per @IsakNaslundBh to follow as a separate issue.

@IsakNaslundBh
Copy link
Contributor

Tested with testfile, now works for me as well!

@IsakNaslundBh IsakNaslundBh merged commit 760567d into master Aug 23, 2019
@IsakNaslundBh IsakNaslundBh deleted the ETABS_Toolkit-BarUniformlyDistributedLoad-Fixes branch August 23, 2019 06:52
@FraserGreenroyd FraserGreenroyd changed the title ETABS Toolkit: Fix read BarUniformlyDistributedLoad Fix read BarUniformlyDistributedLoad Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants