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

[Sofa.Core] Factorize mstate access #2438

Merged
merged 18 commits into from Nov 10, 2021

Conversation

alxbilger
Copy link
Contributor

@alxbilger alxbilger commented Oct 20, 2021

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:

  • All components inherit from StateAccessor, which contains a dynamic list of BaseMechanicalState
  • Components having access to only one mechanical state inherit from SingleStateAccessor. It is templated on DataTypes, with a link to a MechanicalState<DataTypes>.
  • Components having access to 2 mechanical states inherit from PairStateAccessor. It is templated on DataTypes1 and DataTypes2, with a link to a MechanicalState<DataTypes1> and a second link to a MechanicalState<DataTypes2>.
  • Components inheriting from SingleStateAccessor or PairStateAccessor fill the dynamic list contained in the StateAccessor implementation.
  • It does not break the previous accesses, variable names etc.

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 from StateAccessor, 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 from ForceField which has a link to a mechanical state. Therefore, we couldn't have access to a mechanical state from a BaseForceField.

I also cleaned the files:

  • Concatenate nested namespaces
  • Use #pragma once
  • Some private and non-defined copy constructors and operator= are marked delete.
  • Move methods in cpp
  • Removed unnecessary init() implementations. They only called their base class init().
  • Constraint has a link to the mechanical state instead of a raw pointer.
  • Some constructors marked 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

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).

@alxbilger alxbilger added pr: clean Cleaning the code pr: status to review To notify reviewers to review this pull-request labels Oct 20, 2021
@alxbilger
Copy link
Contributor Author

[ci-build][with-all-tests]

@hugtalbot hugtalbot added the pr: breaking Change possibly inducing a compilation error label Nov 3, 2021
@hugtalbot
Copy link
Contributor

would there be a way to separate the cleaning and the changes actually related to the PR?
is it not a problem to add a new class (an interface) to inherit from? does it not make the inheritance tree heavier? This second point is just a remark but obviously not blocking at all

@guparan
Copy link
Contributor

guparan commented Nov 3, 2021

Is it normal that StateAccessor, SingleStateAccessor and PairStateAccessor can be instantiated?
It seems to me that they should be abstract but I may have misunderstood something.

@hugtalbot hugtalbot added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Nov 3, 2021
@alxbilger
Copy link
Contributor Author

is it not a problem to add a new class (an interface) to inherit from? does it not make the inheritance tree heavier? This second point is just a remark but obviously not blocking at all

@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.

@alxbilger
Copy link
Contributor Author

Is it normal that StateAccessor, SingleStateAccessor and PairStateAccessor can be instantiated? It seems to me that they should be abstract but I may have misunderstood something.

@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 ? SOFA_ABSTRACT_CLASS macro or adding a pure virtual destructor (or both)?

To me, only SOFA_ABSTRACT_CLASS makes sense. I am not for adding pure virtual methods. What's your opinion on this @fredroy ?

@alxbilger
Copy link
Contributor Author

would there be a way to separate the cleaning and the changes actually related to the PR?

@hugtalbot not easily. It would take some time. Does it prevent you guys to review the PR?

@fredroy
Copy link
Contributor

fredroy commented Nov 8, 2021

@alxbilger yeah SOFA_ABSTRACT_CLASS was made for this kind of use.
Like BaseForcefield, BaseMapping, etc

@alxbilger alxbilger added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Nov 8, 2021
@alxbilger
Copy link
Contributor Author

[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. "
Copy link
Contributor

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

Copy link
Contributor Author

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.

@fredroy fredroy added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Nov 10, 2021
@fredroy fredroy merged commit e06055d into sofa-framework:master Nov 10, 2021
@guparan guparan added this to the v21.12 milestone Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: breaking Change possibly inducing a compilation error pr: clean Cleaning the code pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants