[SofaKernel] Change the way projective constraints are propagated in visitors to fix problem (BREAKING) #216

guparan · 2017-03-23T15:09:14Z

Description

This PR removes option to apply projective constraints during velocity and/or position propagation visitors, as it makes it unreliable to put projective constraints in child nodes (required when they apply on the DOFs but with a different/subset topology). All codes (solvers and animationloop) must now explicitly call projection operations/visitors before propagations when required (mostly after OdeSolver::solve())

This PR is changing the behavior as projective constraints because they are no longer applied at the end of Simulation::init(). To clearly make that visible to calling's code the propagate visitor was renamed to make sure existing codes fail at compilation time.
To fix that in private/external repos should be easy:

if projective constraints should not be applied, simply apply the rename to the visitor class
if projective constraints should be applied, add a MechanicalProject???Visitor first, then rename the existing one.

CHANGELOG for @guparan and @hugtalbot

[SofaKernel] Change the way all MechanicaVisitor apply projective constraints. This change is breaking the API aand external solver's code need to be updated (see #PR26).
...to finish ....

This PR:

builds with SUCCESS for all platforms on the CI.
does not generate new warnings nor unit test failures.
does not break existing scenes.
does not break API compatibility.
has been reviewed and agreed to be transitional.
is more than 1 week old.

Reviewers will merge only if all these checks are true.

…y and/or position propagation visitors, as it makes it unreliable to put projective constraints in child nodes (required when they apply on the DOFs but with a different/subset topology). All codes (solvers and animationloop) must now explicitly call projection operations/visitors before propagations when required (mostly after OdeSolver::solve())

See 81d1e4d

matthieu-nesme · 2017-03-24T09:03:23Z

SofaKernel/framework/sofa/simulation/AnimateVisitor.cpp

@@ -190,6 +190,9 @@ Visitor::Result AnimateVisitor::processNodeTopDown(simulation::Node* node)
            end(node, node->solver[i], t0);
        }

+        MechanicalProjectPositionAndVelocityVisitor(&m_mparams, nextTime,


It looks like a wrong code duplication.

please keep the #ifdef SOFA_SUPPORT_MAPPED_MASS version.

matthieu-nesme · 2017-03-24T09:05:12Z

SofaKernel/framework/sofa/simulation/MechanicalVisitor.cpp

@@ -491,6 +491,9 @@ Visitor::Result MechanicalIntegrationVisitor::fwdOdeSolver(simulation::Node* nod
    //cerr<<"MechanicalIntegrationVisitor::fwdOdeSolver end solve obj"<<endl;
    //obj->propagatePositionAndVelocity(nextTime,core::VecCoordId::position(),core::VecDerivId::velocity());

+    MechanicalProjectPositionAndVelocityVisitor(&mparams, nextTime,VecCoordId::position(),VecDerivId::velocity()


duplicated code?

matthieu-nesme · 2017-03-24T09:10:07Z

SofaKernel/framework/sofa/simulation/MechanicalVisitor.h

@@ -1333,15 +1372,12 @@ class SOFA_SIMULATION_CORE_API MechanicalPropagatePositionVisitor : public Mecha
    SReal t;
    sofa::core::MultiVecCoordId x;
    bool ignoreMask;
-    bool applyProjections;


ok to remove all the applyProjections from all the propagate visitors.
But a doc should clearly state to run the new visitor MechanicalProjectPositionAndVelocityVisitor before propagation.

I guess there are cases where we want to project only one vector (coord or deriv), but it would be easy to write other visitors for that.

damienmarchal · 2017-03-31T07:36:58Z

[ci-build]

guparan · 2017-04-06T08:47:28Z

@matthieu-nesme: After STC#3 discussions, could you update your review?

matthieu-nesme · 2017-04-12T09:43:17Z

Should be ok with Compliant now.
If everything compiles, the PR is ready to be merged.

matthieu-nesme · 2017-04-12T16:47:30Z

Some tests are now failing, it has to be understood before merging.

hugtalbot · 2017-06-07T09:14:56Z

[ci-build]

…Only\1Visitor to force existing codes to be reviewed at compile time to check if MechanicalProject\1Visitor should be called first. This is safer than risking breakage at runtime for code expecting the old behavior of these visitors. FIX: add missing project visitors in Simulation::init() DOC: update comment of the propagate visitors to explicitly indicate that they do not apply projections anymore.

JeremieA · 2017-06-13T22:42:09Z

The failing tests were mainly due to a unintended side effect that projective constraints were no longer applied at the end of Simulation::init(). To make sure similar issues were not hidden somewhere else, in the last commit I renamed the propagate visitor to make sure existing codes fail at compilation time. Fixing other codes in private/external repos should be easy:

if projective constraints should not be applied, simply apply the rename to the visitor class
if projective constraints should be applied, add a MechanicalProject???Visitor first, then rename the existing one.

This information should be added to the changelog, but as far as I understood this needs to be done after the PR is merged.

JeremieA · 2017-06-13T23:48:43Z

[ci-build]

damienmarchal · 2017-06-14T09:24:46Z

Thanks Jérémie,

The remaining failing tests are our classic failures so this PR seems ok to me.
@guparan Can this one be set to the flag ready for merge ?

damienmarchal · 2017-06-19T10:43:49Z

Is this PR ready ? Should we merge it now ?
[ci-build]

JeremieA · 2017-06-20T11:19:16Z

Yes it is ready as far as I know.

damienmarchal · 2017-06-20T11:20:35Z

@guparan I let you merge this one :)

damienmarchal · 2017-06-26T08:49:10Z

Well...can someone explain briefly how this PR will affect other person's code, how will they be award something have changed and that they need to update something ?

JeremieA · 2017-06-26T09:30:37Z

@damienmarchal : what you are asking is answered in the PR description, and in my comment above, explaining that now everyone will know if they need to update their code at compile-time, and decide what to do based on what I wrote. Anything else is needed ?

damienmarchal · 2017-06-26T10:02:25Z

Hello @JeremieA, the current approach about the changelog management is to have a description of the changes in the PR's description so that @guparan & @hugtalbot can easily integrate them into the CHANGELOG.md file in a (more or less :)) weekly basis.

You can add a dedicated CHANGELOG section in the PR description in which there is a summary of the changes and, in case of behavior or API changes, the mecanism used to notify other's and minimal guideline/example on how they should fix their code. You already provided this informations into the comments feeds...but, having that in the comments feed makes them hard to find so it is better if all that is summarized.

EDIT: I just updated the PR description up to my understanding.

JeremieA · 2017-06-26T10:14:53Z

We edit it at the same time so I lost my contribution...
Will try again later

damienmarchal · 2017-06-26T10:17:19Z

Sorry for the dual edditing... I didn't knew github was not robust against that.

I merge the PR because I hate having PR that longs for month and we can still fix the description even when it is merged/closed.

Many thanks,

matthieu-nesme · 2017-06-28T16:01:43Z

applications/plugins/Flexible/deformationMapping/BaseDeformationMapping.inl

        for(size_t j=0; j<jacobian[i].size(); j++)
        {
            size_t index=indices[i][j];
            jacobian[i][j].addapply(out[i],in[index]);
+            if (i == 0) serr << "out["<<i<<"] + jacobian["<<i<<"]["<<j<<"].addapply(out["<<i<<"],in["<<index<<"]) = " << out[i] << sendl;


JeremieA and others added 3 commits October 18, 2016 16:36

[SofaKernel] FIX missing solveConstraint Data.

b2c6e65

See 81d1e4d

Merge remote-tracking branch 'origin/master' into issofa_visitors

6887250

guparan added the pr: breaking Change possibly inducing a compilation error label Mar 23, 2017

guparan assigned matthieu-nesme, fredroy, guparan and JeremieA Mar 23, 2017

guparan changed the title ~~[Visitors] BREAKING: ISSofa merge~~ [SofaKernel] BREAKING: ISSofa merge: Visitors Mar 23, 2017

guparan changed the title ~~[SofaKernel] BREAKING: ISSofa merge: Visitors~~ [SofaKernel] ISSofa merge: Visitors Mar 23, 2017

guparan added the enhancement About a possible enhancement label Mar 23, 2017

matthieu-nesme suggested changes Mar 24, 2017

View reviewed changes

damienmarchal unassigned matthieu-nesme, fredroy and JeremieA Apr 5, 2017

matthieu-nesme approved these changes Apr 6, 2017

View reviewed changes

matthieu-nesme added 2 commits April 12, 2017 11:01

Merge remote-tracking branch 'upstream/master' into issofa_visitors

ed7f004

[Compliant] updated to the new visitors

3e5ceaa

hugtalbot added the pr: status wip Development in the pull-request is still in progress label May 12, 2017

JeremieA added 2 commits June 7, 2017 15:35

Merge branch 'master' into issofa_visitors

6ad81d2

JeremieA removed the pr: status wip Development in the pull-request is still in progress label Jun 20, 2017

damienmarchal changed the title ~~[SofaKernel] ISSofa merge: Visitors~~ [SofaKernel] Change the way projective constraints are propagated in visitors to fix problem (BREAKING) Jun 26, 2017

damienmarchal merged commit 05cb669 into master Jun 26, 2017

matthieu-nesme reviewed Jun 28, 2017

View reviewed changes

guparan added this to the v17.12 milestone Jun 29, 2017

guparan deleted the issofa_visitors branch September 18, 2020 12:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SofaKernel] Change the way projective constraints are propagated in visitors to fix problem (BREAKING) #216

[SofaKernel] Change the way projective constraints are propagated in visitors to fix problem (BREAKING) #216

guparan commented Mar 23, 2017 •

edited by damienmarchal

Loading

matthieu-nesme Mar 24, 2017

matthieu-nesme Mar 24, 2017

matthieu-nesme Mar 24, 2017

damienmarchal commented Mar 31, 2017

guparan commented Apr 6, 2017

matthieu-nesme commented Apr 12, 2017

matthieu-nesme commented Apr 12, 2017

hugtalbot commented Jun 7, 2017

JeremieA commented Jun 13, 2017

JeremieA commented Jun 13, 2017

damienmarchal commented Jun 14, 2017 •

edited

Loading

damienmarchal commented Jun 19, 2017

JeremieA commented Jun 20, 2017

damienmarchal commented Jun 20, 2017

damienmarchal commented Jun 26, 2017 •

edited

Loading

JeremieA commented Jun 26, 2017

damienmarchal commented Jun 26, 2017 •

edited

Loading

JeremieA commented Jun 26, 2017

damienmarchal commented Jun 26, 2017

matthieu-nesme Jun 28, 2017

[SofaKernel] Change the way projective constraints are propagated in visitors to fix problem (BREAKING) #216

[SofaKernel] Change the way projective constraints are propagated in visitors to fix problem (BREAKING) #216

Conversation

guparan commented Mar 23, 2017 • edited by damienmarchal Loading

Description

matthieu-nesme Mar 24, 2017

Choose a reason for hiding this comment

matthieu-nesme Mar 24, 2017

Choose a reason for hiding this comment

matthieu-nesme Mar 24, 2017

Choose a reason for hiding this comment

damienmarchal commented Mar 31, 2017

guparan commented Apr 6, 2017

matthieu-nesme commented Apr 12, 2017

matthieu-nesme commented Apr 12, 2017

hugtalbot commented Jun 7, 2017

JeremieA commented Jun 13, 2017

JeremieA commented Jun 13, 2017

damienmarchal commented Jun 14, 2017 • edited Loading

damienmarchal commented Jun 19, 2017

JeremieA commented Jun 20, 2017

damienmarchal commented Jun 20, 2017

damienmarchal commented Jun 26, 2017 • edited Loading

JeremieA commented Jun 26, 2017

damienmarchal commented Jun 26, 2017 • edited Loading

JeremieA commented Jun 26, 2017

damienmarchal commented Jun 26, 2017

matthieu-nesme Jun 28, 2017

Choose a reason for hiding this comment

guparan commented Mar 23, 2017 •

edited by damienmarchal

Loading

damienmarchal commented Jun 14, 2017 •

edited

Loading

damienmarchal commented Jun 26, 2017 •

edited

Loading

damienmarchal commented Jun 26, 2017 •

edited

Loading