[all] Sofa add mechanical matrix mapper #721

olivier-goury · 2018-07-18T15:26:31Z

This PR:

adds the new component MechanicalMatrixMapper (formerly called MappedMatrixForceField)
suppress the Cmake option SOFA_WITH_EXPERIMENTAL_FEATURES (Was used for development and is useless now) added in Mapped forcefield matrix + minor spelling changes #276

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:

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

Reviewers will merge only if all these checks are true.

Corresponding changes are now on the branch removed_renumberConstraintId and should be merged to SOFA using PR process This reverts commit 3ca5df3.

…onLoop

hugtalbot · 2018-07-18T15:41:44Z

[ci-build][with-scene-tests]

epernod

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?

epernod

Hi nice one!
As this new component is very technical, could you provide at least one scene example in /examples/Components/animationloop/MechanicalMatrixMapper.scn

epernod · 2018-07-19T11:16:05Z

modules/SofaGeneralAnimationLoop/MechanicalMatrixMapper.inl

+
+
+template<class DataTypes1, class DataTypes2>
+void MechanicalMatrixMapper<DataTypes1, DataTypes2>::addKToMatrix(const MechanicalParams* mparams,


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?

epernod · 2018-07-19T11:17:36Z

modules/SofaGeneralAnimationLoop/MechanicalMatrixMapper.inl

+{
+    sofa::core::behavior::BaseInteractionForceField::init();
+
+    if (mstate1.get() == NULL || mstate2.get() == NULL)


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;

Thanks for the remarks, added in commit 451aefe

guparan · 2018-07-20T09:53:17Z

modules/SofaGeneralAnimationLoop/MechanicalMatrixMapper.cpp

@@ -0,0 +1,77 @@
+/******************************************************************************
+*       SOFA, Simulation Open-Framework Architecture, version 1.0 RC 1        *
+*                (c) 2006-2011 MGH, INRIA, USTL, UJF, CNRS                    *


2006-2018 ;-)

Done with ac71b0e, thanks for the review!

guparan · 2018-07-20T09:53:40Z

modules/SofaGeneralAnimationLoop/MechanicalMatrixMapper.h

@@ -0,0 +1,222 @@
+/******************************************************************************
+*       SOFA, Simulation Open-Framework Architecture, version 1.0 RC 1        *
+*                (c) 2006-2011 MGH, INRIA, USTL, UJF, CNRS                    *


2006-2018 ;-)

guparan · 2018-07-20T09:53:50Z

modules/SofaGeneralAnimationLoop/MechanicalMatrixMapper.inl

@@ -0,0 +1,543 @@
+/******************************************************************************
+*       SOFA, Simulation Open-Framework Architecture, version 1.0 RC 1        *
+*                (c) 2006-2011 MGH, INRIA, USTL, UJF, CNRS                    *


2006-2018 ;-)

…ofaDefrost/sofa into Sofa_addMechanicalMatrixMapper

olivier-goury · 2018-07-20T15:47:51Z

@epernod Thank you for your comments!
When using a direct solver (e.g. SparseLDLSolver), and when the SOFA scene involves a forcefield under a mapping, this component allows to map the stiffness matrix through this mapping (and also the Mass matrix). At the moment SOFA just ignores that.

epernod · 2018-07-22T20:07:10Z

ok but shouldn't be the mapping that automatically does that?

olivier-goury · 2018-07-23T09:06:09Z

Hey Erik,
You're right, this component is a current fix for the issue of mapping matrices through mappings, but ideally it would be an operation called automatically by the animation loop and done by the mapping for example. We will look into that in the future.
Olivier

damienmarchal · 2018-07-25T08:49:02Z

modules/SofaGeneralAnimationLoop/MechanicalMatrixMapper.cpp

+////////////////////////////////////////////    FACTORY    //////////////////////////////////////////////
+SOFA_DECL_CLASS(MechanicalMatrixMapper)
+
+int MechanicalMatrixMapperClass = core::RegisterObject("Partially rigidify a mechanical object using a rigid mapping.")


Is this description correct ?

No it's not!

olivier-goury · 2018-07-25T09:58:42Z

Doxygen doc was added to the component

damienmarchal · 2018-07-25T11:55:51Z

SofaKernel/modules/SofaBaseMechanics/MechanicalObject.inl

+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;


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 ?

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.

I agree with your Guillaume :)

damienmarchal · 2018-07-25T12:00:52Z

examples/Components/animationloop/MechanicalMatrixMapperExample.pyscn

+        FEMNode= deformablePartNode.createChild('FEMNode')
+        RigidifiedNode.addChild(FEMNode)
+
+        FEMNode.createObject('Mesh',name='meshInput',src="@../../../../meshDivision/beamMesh")                     


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.

damienmarchal · 2018-07-25T12:01:32Z

examples/Components/animationloop/MechanicalMatrixMapperExample.pyscn

+        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')


Don't use "0.1" to pass constants you can just write totalmass=1.0

damienmarchal · 2018-07-25T12:02:10Z

examples/Components/animationloop/MechanicalMatrixMapperExample.pyscn

+        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')


Remove useless lines :)

damienmarchal · 2018-07-25T12:02:31Z

examples/Components/animationloop/MechanicalMatrixMapperExample.pyscn

@@ -0,0 +1,50 @@
+import Sofa
+import random	


Is 'random' of any use ?

damienmarchal · 2018-07-25T12:46:28Z

modules/SofaGeneralAnimationLoop/MechanicalMatrixMapper.inl

+    }
+    else
+    {
+        msg_info(this) << "There is no mappedMass";


Update doxygen doc in the header.

olivier-goury · 2018-07-26T08:34:24Z

Doxygen doc has been added in the header file
Html doc was added in the location of the example in examples/Components/animationLoop

olivier-goury · 2018-07-27T08:08:19Z

Hello,
What is the status for this pull request? Are you OK to merge it?

guparan · 2018-07-27T13:35:44Z

Hi @olivier-goury,
I see there still are some edit requests from @damienmarchal in the comments.
Do you plan to fix them?

damienmarchal · 2018-07-27T13:54:30Z

Actually there is no doubt this needs to be edited because it contains files that are not related to the PR.

olivier-goury · 2018-07-27T14:26:52Z

OK, I removed changes unrelated to this PR, so there are no more unfulfilled edit requests!

damienmarchal · 2018-08-29T11:28:59Z

[ci-build][with-scene-tests]

olivier-goury · 2018-08-30T08:13:28Z

Hello, do you have more remarks on this PR?
Could we consider going for the merge?
Thanks

damienmarchal · 2018-09-03T09:03:19Z

Not for me :)

EulalieCoevoet and others added 6 commits July 5, 2018 09:57

Revert "Remove unused function "renumberConstraintId""

e210b53

Corresponding changes are now on the branch removed_renumberConstraintId and should be merged to SOFA using PR process This reverts commit 3ca5df3.

Merge remote-tracking branch 'upstream/master'

81a609b

Merge remote-tracking branch 'upstream/master'

ace853d

[simulation] ADD MechanicalMatrixMapper

9d33c87

Remove option SOFA_WITH_ESPERIMENTAL_FEATURES

ca0dd44

[simulation] Move MechanicalMatrixMapper to module SofaGeneralAnimati…

56e37c8

…onLoop

hugtalbot changed the title ~~Sofa add mechanical matrix mapper~~ [all] Sofa add mechanical matrix mapper Jul 18, 2018

hugtalbot added enhancement About a possible enhancement pr: status to review To notify reviewers to review this pull-request labels Jul 18, 2018

[SofaGeneralAnimationLoop] RM useless includes

3d0b261

epernod requested changes Jul 19, 2018

View reviewed changes

[SofaGeneralAnimationLoop] Add test to init on component links

451aefe

guparan suggested changes Jul 20, 2018

View reviewed changes

[SofaGeneralAnimationLoop] Fix license date

ac71b0e

guparan approved these changes Jul 20, 2018

View reviewed changes

olivier-goury and others added 3 commits July 20, 2018 17:34

Add example using MechanicalMatrixMapper

0452e53

[SofaGeneralAnimationLoop] Fix problem at init

0c4a6dd

Merge branch 'Sofa_addMechanicalMatrixMapper' of https://github.com/S…

5ef9872

…ofaDefrost/sofa into Sofa_addMechanicalMatrixMapper

epernod approved these changes Jul 22, 2018

View reviewed changes

damienmarchal reviewed Jul 25, 2018

View reviewed changes

guparan 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 Jul 25, 2018

Update documentation and example location for MechanicalMatrixMapper

419ebf9

damienmarchal reviewed Jul 25, 2018

View reviewed changes

VannesteFelix and others added 2 commits July 25, 2018 18:11

[SofaGeneralAnimationLoop] Clean MechanicalMatrixMapper

a14acf2

Add doc file next to MechanicalMatrixMapper example.

ed477cb

Update doxygen doc in the header.

Remove renumberConstraintID modifications

05d3098

olivier-goury 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 Aug 29, 2018

[SofaFramework] Delete useless file

af9f166

guparan 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 Sep 5, 2018

guparan merged commit 577716d into sofa-framework:master Sep 6, 2018

olivier-goury deleted the Sofa_addMechanicalMatrixMapper branch September 12, 2018 15:15

guparan added this to the v18.12 milestone Oct 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[all] Sofa add mechanical matrix mapper #721

[all] Sofa add mechanical matrix mapper #721

olivier-goury commented Jul 18, 2018 •

edited by guparan

Loading

hugtalbot commented Jul 18, 2018

epernod left a comment •

edited

Loading

epernod left a comment

epernod Jul 19, 2018 •

edited

Loading

epernod Jul 19, 2018

VannesteFelix Jul 19, 2018

guparan Jul 20, 2018

VannesteFelix Jul 20, 2018

guparan Jul 20, 2018

VannesteFelix Jul 20, 2018

guparan Jul 20, 2018

VannesteFelix Jul 20, 2018

olivier-goury commented Jul 20, 2018

epernod commented Jul 22, 2018

olivier-goury commented Jul 23, 2018

damienmarchal Jul 25, 2018 •

edited

Loading

olivier-goury Jul 25, 2018

olivier-goury commented Jul 25, 2018

damienmarchal Jul 25, 2018 •

edited

Loading

guparan Jul 27, 2018

damienmarchal Jul 27, 2018

damienmarchal Jul 25, 2018

damienmarchal Jul 25, 2018

damienmarchal Jul 25, 2018

damienmarchal Jul 25, 2018 •

edited

Loading

damienmarchal Jul 25, 2018

olivier-goury commented Jul 26, 2018

olivier-goury commented Jul 27, 2018

guparan commented Jul 27, 2018

damienmarchal commented Jul 27, 2018

olivier-goury commented Jul 27, 2018

damienmarchal commented Aug 29, 2018

olivier-goury commented Aug 30, 2018

damienmarchal commented Sep 3, 2018



		template<class DataTypes1, class DataTypes2>
		void MechanicalMatrixMapper<DataTypes1, DataTypes2>::addKToMatrix(const MechanicalParams* mparams,

[all] Sofa add mechanical matrix mapper #721

[all] Sofa add mechanical matrix mapper #721

Conversation

olivier-goury commented Jul 18, 2018 • edited by guparan Loading

hugtalbot commented Jul 18, 2018

epernod left a comment • edited Loading

Choose a reason for hiding this comment

epernod left a comment

Choose a reason for hiding this comment

epernod Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivier-goury commented Jul 20, 2018

epernod commented Jul 22, 2018

olivier-goury commented Jul 23, 2018

damienmarchal Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivier-goury commented Jul 25, 2018

damienmarchal Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damienmarchal Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivier-goury commented Jul 26, 2018

olivier-goury commented Jul 27, 2018

guparan commented Jul 27, 2018

damienmarchal commented Jul 27, 2018

olivier-goury commented Jul 27, 2018

damienmarchal commented Aug 29, 2018

olivier-goury commented Aug 30, 2018

damienmarchal commented Sep 3, 2018

olivier-goury commented Jul 18, 2018 •

edited by guparan

Loading

epernod left a comment •

edited

Loading

epernod Jul 19, 2018 •

edited

Loading

damienmarchal Jul 25, 2018 •

edited

Loading

damienmarchal Jul 25, 2018 •

edited

Loading

damienmarchal Jul 25, 2018 •

edited

Loading