-
Notifications
You must be signed in to change notification settings - Fork 311
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
[Sofa.Core] Factorize mstate access #2438
[Sofa.Core] Factorize mstate access #2438
Conversation
…access # Conflicts: # SofaKernel/modules/SofaCore/src/sofa/core/behavior/Mass.h
[ci-build][with-all-tests] |
would there be a way to separate the cleaning and the changes actually related to the PR? |
Is it normal that StateAccessor, SingleStateAccessor and PairStateAccessor can be instantiated? |
@hugtalbot the answer is not simple (see https://stackoverflow.com/questions/7210412/what-is-the-cost-of-inheritance for example). However, in our case, I think the cost (if any) is negligible. |
@guparan You are right that it does not make sense to instantiate those classes. However, they do not have any virtual method. What mechanism did you have in mind ? To me, only |
@hugtalbot not easily. It would take some time. Does it prevent you guys to review the PR? |
@alxbilger yeah SOFA_ABSTRACT_CLASS was made for this kind of use. |
[ci-build][with-all-tests] |
{ | ||
mstate.set(dynamic_cast< MechanicalState<DataTypes>* >(getContext()->getMechanicalState())); | ||
|
||
msg_error_when(!mstate) << "No compatible MechanicalState found in the current context. " |
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.
maybe the msg should only be a msg_error()
?? @alxbilger
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.
you mean because there is no need to check "again" if mstate is null? Actually, it is not redundant. It is to verify that the search in the context returns something valid.
Most of the components in
Sofa.Core/behavior
work by accessing one or more mechanical states, in order to access state variables, such as position, force etc. They all manage their own way to store, find and manage their link to one or more mechanical states. Instead, I suggest to factorize this access using intermediate classes. It comes with the following features:StateAccessor
, which contains a dynamic list ofBaseMechanicalState
SingleStateAccessor
. It is templated onDataTypes
, with a link to aMechanicalState<DataTypes>
.PairStateAccessor
. It is templated onDataTypes1
andDataTypes2
, with a link to aMechanicalState<DataTypes1>
and a second link to aMechanicalState<DataTypes2>
.SingleStateAccessor
orPairStateAccessor
fill the dynamic list contained in theStateAccessor
implementation.The goal is that all the components accessing a
BaseMechanicalState
can provide a list of them in a generic way. For example,BaseInteractionForceField
supposed only 2 mechanical states. Since it inherits now fromStateAccessor
, it can have links to more than 2 (note that none of the interaction force field I found work with more than 2). Another example:BaseForceField
did not have any reference to a mechanical state, but all the force fields inherits fromForceField
which has a link to a mechanical state. Therefore, we couldn't have access to a mechanical state from aBaseForceField
.I also cleaned the files:
#pragma once
operator=
are markeddelete
.init()
implementations. They only called their base classinit()
.Constraint
has a link to the mechanical state instead of a raw pointer.explicit
By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).
Reviewers will merge this pull-request only if