-
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
[all] Sofa add mechanical matrix mapper #721
[all] Sofa add mechanical matrix mapper #721
Conversation
Corresponding changes are now on the branch removed_renumberConstraintId and should be merged to SOFA using PR process This reverts commit 3ca5df3.
[ci-build][with-scene-tests] |
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'm sorry, I'm not an expert in this.
Is this new mapper a way to optimise the computation of the mechanical system or is it a new mechanism that was not possible before?
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.
Hi nice one!
As this new component is very technical, could you provide at least one scene example in /examples/Components/animationloop/MechanicalMatrixMapper.scn
|
||
|
||
template<class DataTypes1, class DataTypes2> | ||
void MechanicalMatrixMapper<DataTypes1, DataTypes2>::addKToMatrix(const MechanicalParams* mparams, |
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.
This one is quite a big process. Do you think it could be possible to add a unit test for this method with known results?
{ | ||
sofa::core::behavior::BaseInteractionForceField::init(); | ||
|
||
if (mstate1.get() == NULL || mstate2.get() == NULL) |
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 are testing it well here. But there are several access to the mstate1 and 2 in the code. How will the scene behave if mstate are not found?
maybe you could use the paremeter: ComponentState::Invalid;
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.
Thanks for the remarks, added in commit 451aefe
@@ -0,0 +1,77 @@ | |||
/****************************************************************************** | |||
* SOFA, Simulation Open-Framework Architecture, version 1.0 RC 1 * | |||
* (c) 2006-2011 MGH, INRIA, USTL, UJF, CNRS * |
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.
2006-2018 ;-)
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.
Done with ac71b0e, thanks for the review!
@@ -0,0 +1,222 @@ | |||
/****************************************************************************** | |||
* SOFA, Simulation Open-Framework Architecture, version 1.0 RC 1 * | |||
* (c) 2006-2011 MGH, INRIA, USTL, UJF, CNRS * |
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.
2006-2018 ;-)
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.
Done
@@ -0,0 +1,543 @@ | |||
/****************************************************************************** | |||
* SOFA, Simulation Open-Framework Architecture, version 1.0 RC 1 * | |||
* (c) 2006-2011 MGH, INRIA, USTL, UJF, CNRS * |
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.
2006-2018 ;-)
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.
Done
@epernod Thank you for your comments! |
ok but shouldn't be the mapping that automatically does that? |
Hey Erik, |
//////////////////////////////////////////// FACTORY ////////////////////////////////////////////// | ||
SOFA_DECL_CLASS(MechanicalMatrixMapper) | ||
|
||
int MechanicalMatrixMapperClass = core::RegisterObject("Partially rigidify a mechanical object using a rigid mapping.") |
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.
Is this description correct ?
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.
No it's not!
Doxygen doc was added to the component |
template <class DataTypes> | ||
void MechanicalObject<DataTypes>::renumberConstraintId(const sofa::helper::vector<unsigned>& /*renumbering*/) | ||
{ | ||
this->serr << "MechanicalObject<DataTypes>::renumberConstraintId not implemented in the MatrixDeriv constraint API" << this->sendl; |
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.
Please don't use serr/sout anymore...
but more fundamentally... a pure virtual method is ...pure specifically to force the object inheriting from the parent to not forget to implement them. Providing an empty implementation like this one sounds like skipping over the safeguard that was put in place so it compile and let the user handle the problem (which I doubt it can).
Don't we have a better way to handle those cases ?
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.
If the override is mandatory, a pure virtual function is the way to go. This compiler-bypassing behavior should not be allowed.
If the override is not mandatory, then non-virtual + msg_warning("You didn't override") seems nice.
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 agree with your Guillaume :)
FEMNode= deformablePartNode.createChild('FEMNode') | ||
RigidifiedNode.addChild(FEMNode) | ||
|
||
FEMNode.createObject('Mesh',name='meshInput',src="@../../../../meshDivision/beamMesh") |
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 should replace link made by string like "@../../../../meshDivision/beamMesh" with automatically generated link from the target object eg: "@"+meshOfStructure.getPathName()
It is more robust against change of location & naming of the targeted components.
FEMNode.createObject('MechanicalObject', template='Vec3d',name='beamMecha') | ||
FEMNode.createObject('HexahedronFEMForceField', name='HexaFF', src="@meshInput") | ||
FEMNode.createObject('RestShapeSpringsForceField', name='restShapeFF', points='@../../../meshDivision/box_roi_fix.indices', stiffness="10000", angularStiffness="10000") | ||
FEMNode.createObject('UniformMass', totalmass='0.1') |
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.
Don't use "0.1" to pass constants you can just write totalmass=1.0
SolverNode= rootNode.createChild('SolverNode') | ||
SolverNode.createObject('EulerImplicit',verbose='false') | ||
SolverNode.createObject('SparseLDLSolver', name="ldlsolveur") | ||
#SolverNode.createObject('MechanicalMatrixMapper', template='Vec3d,Rigid', object1='@./deformablePartNode/beamPart1Mech', object2='@./RigidNode/rigid1', nodeToParse='@./deformablePartNode/FEMNode') |
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.
Remove useless lines :)
@@ -0,0 +1,50 @@ | |||
import Sofa | |||
import random |
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.
Is 'random' of any use ?
} | ||
else | ||
{ | ||
msg_info(this) << "There is no mappedMass"; |
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.
no this.
Update doxygen doc in the header.
|
Hello, |
Hi @olivier-goury, |
Actually there is no doubt this needs to be edited because it contains files that are not related to the PR. |
OK, I removed changes unrelated to this PR, so there are no more unfulfilled edit requests! |
[ci-build][with-scene-tests] |
Hello, do you have more remarks on this PR? |
Not for me :) |
This PR:
The component MechanicalMatrixMapper allows to compute the mapped matrices computed by ForceFields under mappings, which are ignored by the SOFA animation loops.
Typical usage would be:
Node
MechanicalObject (MO) q
MechanicalMatrixMapper (NodeParsed = MappedNode) --> computes J^T.K.J and J^T.M.J
.....MappedNode
.....MappedMechanicalObject (MMO) q_m
.....Mapping (input = ../MO, output = MMO) -> Computes J.q_m
.....ForceField -> Computes stiffness matrix K
.....Mass -> Computes Mass Matrix M
Any comments welcome!
(Work with @VannesteFelix @ChristianDuriez )
This PR:
Reviewers will merge only if all these checks are true.