-
Notifications
You must be signed in to change notification settings - Fork 273
[IGA] New Isogeometric Reissner-Mindlin Shell Element #13933
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks! |
matekelemen
left a comment
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.
Suggestions on cleaning up the header.
| /// Short class definition. | ||
| /** Reissner-Mindlin shell based on degenerated solid (Benson et al.) | ||
| */ |
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.
The docs need a bit more love. The minimum is a DOI for the source, but it'd be great if a short summary was made available here about the most important properties of the element without having to scour the web and read a whole paper just to get that.
| protected: | ||
|
|
||
| /// Internal variables used for metric transformation | ||
| struct KinematicVariables |
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.
These internal classes need not be in the header.
| //base vector 1 | ||
| array_1d<double, 3> a1; | ||
| //base vector 2 | ||
| array_1d<double, 3> a2; | ||
| //base vector 3 normalized | ||
| array_1d<double, 3> a3; | ||
| //not-normalized base vector 3 | ||
| array_1d<double, 3> a3_tilde; |
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 can't say I'm enthusiastic about these variable names. If you can't come up with more descriptive names, it's all the more important that the source material, which the naming is based on, is accessible.
| /** | ||
| * Internal variables used in the constitutive equations | ||
| */ | ||
| struct SecondVariations |
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.
None of these internal classes need to be in the header.
| KRATOS_CLASS_INTRUSIVE_POINTER_DEFINITION(Shell6pElement); | ||
|
|
||
| /// Size types | ||
| typedef std::size_t IndexType; |
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.
usingovertypedef- IndexType and SizeType #13389
| /// Destructor. | ||
| virtual ~Shell6pElement() = default; |
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.
| /// Destructor. | |
| virtual ~Shell6pElement() = default; |
| void CalculateRightHandSide( | ||
| VectorType& rRightHandSideVector, | ||
| const ProcessInfo& rCurrentProcessInfo) override | ||
| { |
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.
The definition of all the virtual functions should be in the source. There's no point in keeping the in the header since they're virtual and will never be inlined.
| // Components of the metric coefficient tensor on the contravariant basis | ||
| std::vector<array_1d<double, 3>> m_A_ab_covariant_vector; | ||
| // Components of the curvature coefficient tensor on the contravariant basis | ||
| std::vector<array_1d<double, 3>> m_B_ab_covariant_vector; | ||
|
|
||
| // Determinant of the geometrical Jacobian. | ||
| Vector m_dA_vector; | ||
|
|
||
| /* Transformation the strain tensor from the curvilinear system | ||
| * to the local cartesian in voigt notation including a 2 in the | ||
| * shear part. */ | ||
| std::vector<Matrix> m_T_vector; | ||
|
|
||
| /// The vector containing the constitutive laws for all integration points. | ||
| std::vector<ConstitutiveLaw::Pointer> mConstitutiveLawVector; | ||
|
|
||
| // curvilinear coordinate zeta (theta3) | ||
| double mZeta; |
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.
- private member variables in
Elements will cause a lot of incompatibilities with other Kratos features. I recommend moving these to theElement'sDataValueContainer. - plase keep to the naming style guide
| void CalculateAll( | ||
| MatrixType& rLeftHandSideMatrix, | ||
| VectorType& rRightHandSideVector, | ||
| const ProcessInfo& rCurrentProcessInfo, | ||
| const bool CalculateStiffnessMatrixFlag, | ||
| const bool CalculateResidualVectorFlag | ||
| ); | ||
|
|
||
| /// Initialize Operations | ||
| void InitializeMaterial(); |
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.
As I understand it, all of these functions are for internal use only. If that's the case, move them to the source file. No need to bloat the header.
| const ConstitutiveLaw::StressMeasure ThisStressMeasure | ||
| ) const; | ||
|
|
||
| inline void CalculateAndAddK( |
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.
inline does not do what you think it does. It's completely unnecessary here.
| inline void CalculateAndAddK( | |
| void CalculateAndAddK( |
Also these functions should all be moved to source.
Thanks! Whenever the draft is ready, I will open it and ask you to review it again. |
📝 Description
Implementation of a new isogeometric Reissner-Mindlin shell element based on degenerated solid theory (6p).
The PR is still in draft state (not ready to be merged as major changes are planned).
Credit to @mar0x13 , I have created this draft based on your development.
FYI: @juancamarotti