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

Load, Result and Element fixes #201

Merged
merged 7 commits into from
Sep 12, 2019
Merged

Load, Result and Element fixes #201

merged 7 commits into from
Sep 12, 2019

Conversation

JosefTaylor
Copy link
Contributor

@JosefTaylor JosefTaylor commented Sep 6, 2019 •
edited
Loading

Issues addressed by this PR

Closes #199
Closes #198
Closes #200
Closes #202
Closes #130

Test files

Any old test file for general fixes
for Issue 202

Changelog

  • Set CopyLocal to True for ETABS2016.dll so that fresh installs work.
  • Removed Loadcase.toCSI() and removed from ReadResults.
  • Fixed Read Result - set up cases and combos to get the name of the case if the input is a case, and if the input is a string, assume the string is the case id in ETABS.
  • Removed error returned when loadcase already exists in ETABS.
  • Fixed CreatePanel - If panel property has no name, use None property in ETABS.
  • Fixed CreateBar - if bar property has no name, use None property in ETABS.

@JosefTaylor JosefTaylor added this to the BHoM 2.4 β MVP milestone Sep 6, 2019
@JosefTaylor
Copy link
Contributor Author

If we want to add a couple more fixes to this branch before merging and the MVP, I'm open to it.

IsakNaslundBh
IsakNaslundBh reviewed Sep 9, 2019
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.

Generally looks ok to me after just reading through the code.

Wondering a bit regarding if dead cases always should have a gravity factor of 1, see comment.

Etabs_Adapter/Create/Load.cs Outdated
CreateElementError("Loadcase", loadcase.Name);
}
double selfWeight = 0;
if (loadcase.Nature == LoadNature.Dead)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we want to do this? This will make this adapter behave quite differently to the other ones. Have solved this using the GravityLoad before for Etabs, same as is done in the other adapters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure at all! This was in the ETABS toolkit when I copied the code into SAP, so I figured it was good.
I don't know about GravityLoad- I'll read up and see if I can implement quickly (if it's not done already)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After reviewing, I've removed the selfweight bit from Loadcase.

I also noticed a naming inconsistency: Etabs_Adapter vs. ETABS_Engine. Is this as simple a fix as I think it is? Will anything be impacted by changing the name of Etabs_Adapter to ETABS_Adapter?

Josef Taylor added 2 commits September 9, 2019 10:27
Removed self weight setting
e685ec5
Fixed #202
1304fa4 
If panel property has no name, use None property in ETABS.
kThorsager
kThorsager reviewed Sep 12, 2019
View reviewed changes
Copy link
Contributor

@kThorsager kThorsager left a comment

Choose a reason for hiding this comment

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

I'll start by saying that I don't know ETABS, but this message appears despite materials being applied in ETABS on the slab section property. I don't know why Deck1 appears but it gets an material, but not the right one (if it's suppose to get the same?) But the pushed materials are present in the dropdown list for Deck1
1. Could not get material from ETABS. Material for surface property Deck1 will be null
Not sure what that is about

Then I must say that this: 2. Surface properties with no name will be assigned 'None'. was not initially apparent. I thought that its name was assigned to "None" but in ETABS it seems like it's assigned no surface properties, which is a property named "None" in ETABS. On a closer read I get what it means but it could also be more apparent at a glance like 2. Surface properties with no name will not be assigned.

Then despite the object being pushed and appearing in ETABS the component returns False and the object output is empty, not sure if that is intended with partial failures.
This is when the panel property is nameless

@JosefTaylor JosefTaylor marked this pull request as ready for review September 12, 2019 13:29
@JosefTaylor
Copy link
Contributor Author

JosefTaylor commented Sep 12, 2019 •
edited
Loading

The first bit is a note- when you push surfaces, it has to read the existing surface properties, and any that it doesn't understand it will send back that note. it doesn't matter during push, and I'd like to know a way to suppress the note, but I don't know at this point.
The error message for unnamed properties shouldn't cause a false return, I'll look into that. As to the wording, how about: Surface properties with no name will be converted to the null property 'None'. I'm also thinking, since I understand you're pushing a surface property which is real, i.e. not null, but with no name, that that note should be a warning, so the user gets the urgency of providing a name to ETABS.

Use AdapterId instead of Name to get properties
e34e500 
Added no name check for create bar sections
Changed create bar method to get section by AdapterId
Changed no name note to warning
@JosefTaylor
Copy link
Contributor Author

@kThorsager I've just changed both bar and panel to this behavior, have a look.

@JosefTaylor
Copy link
Contributor Author

@IsakNaslundBh @enarhi please review so I can merge tomorrow.

enarhi
enarhi approved these changes Sep 12, 2019
View reviewed changes
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.

Looks good to me!

@JosefTaylor JosefTaylor merged commit d4abe38 into master Sep 12, 2019
@JosefTaylor JosefTaylor deleted the ETABS_Toolkit-issue199-misc-load-fixes branch September 12, 2019 21:20
@IsakNaslundBh IsakNaslundBh changed the title Etabs toolkit issue199 misc load fixes Load, Result and element fixes Sep 13, 2019
@IsakNaslundBh IsakNaslundBh changed the title Load, Result and element fixes Load, Result and Element fixes Sep 13, 2019
@FraserGreenroyd FraserGreenroyd changed the title Load, Result and Element fixes Load, Result and Element fixesssssss Aug 7, 2023
@FraserGreenroyd FraserGreenroyd changed the title Load, Result and Element fixesssssss Load, Result and Element fixes Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kThorsager kThorsager kThorsager left review comments

@IsakNaslundBh IsakNaslundBh IsakNaslundBh left review comments

@enarhi enarhi enarhi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
BHoM 2.4 β MVP
4 participants
@JosefTaylor @enarhi @IsakNaslundBh @kThorsager