[SofaBoundaryCondition] ADD flag PROJECTVELOCITY #288

adagolodjo · 2017-06-06T09:11:43Z

In component FixeConstraint in function projectVelocity use Data flag instead of #define Flag to set velocity and free velocity to zero.
@digitaltrainers and @matthieu-nesme

Suggested label enhance

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.

sofabot · 2017-06-06T09:11:45Z

Thank you for your pull request!
Someone will soon check it and start the builds.

matthieu-nesme

Ok to add a flag to filter the velocities.

If it is done in FixedConstraint, it also has to be done in PartialFixedConstraint.

Note that in the general case it is not necessary to project the velocity at each time step, it should be sufficient to do so only once when creating/modifying the mstate. (Or even better: never calling it by creating a valid state from the beginning!)

matthieu-nesme · 2017-06-06T09:46:39Z

modules/SofaBoundaryCondition/FixedConstraint.h

@@ -86,6 +87,7 @@ class FixedConstraint : public core::behavior::ProjectiveConstraintSet<DataTypes
    Data<bool> f_fixAll;
    Data<bool> f_showObject;
    Data<SReal> f_drawSize;
+    Data<bool> f_activate_projectVelocity;


I would rename this data as "projectVelocity" (variable d_projectVelocity)

matthieu-nesme · 2017-06-06T09:52:38Z

modules/SofaBoundaryCondition/FixedConstraint.inl

+    const SetIndexArray & indices = this->f_indices.getValue();
+    helper::WriteAccessor<DataVecDeriv> res ( mparams, vData );
+
+    if( this->f_fixAll.getValue()==true )    // fix everyting
    {
        for( unsigned i=0; i<res.size(); i++ )
            res[i] = Deriv();


ok to filter freeVelocity BUT

it is not necessary to do it at each time step

to be coherent it should also be done when all dofs are fixed

the vector freeVelocity is not allocated in the general case

if this projective constraint is applied on freeVelocity, every other projective constraints should also be applied.

The clean way is to call the projectVelocity visitor on freeVelocity where needed (in general in the solver that allocates freeVelocity) but definitively not hard-coded in FixedConstraint::projectVelocity.

guparan · 2017-06-06T12:13:59Z

[ci-build]

damienmarchal · 2017-06-06T13:03:46Z

Hi @younesssss

Thanks for your PR.

I strongly support removing the #define/#ifdef where-ever this is possible because #ifdef leads to code that is very hard to maintain and test in a CI. Your proposition is going toward that so I like it.

Matthieu suggested an alternative way to achieve the same result do you think you can do it or is it too far away from what you had in mind ?

adagolodjo · 2017-06-06T17:50:50Z

Hi @damien-marchal, Yes I will do it. Maybe just need to talk with @matthieu-nesme to understand more his suggestions. Thanks. 2017-06-06 15:03 GMT+02:00 Damien Marchal <notifications@github.com>:

…

Hi @younesssss <https://github.com/younesssss> Thanks for your PR. I strongly support removing the #define/#ifdef where-ever this is possible because #ifdef leads to code that is very hard to maintain and test in a CI. Your proposition is going toward that so I like it. Matthieu suggested an alternative way to achieve the same result do you think you can do it ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#288 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXU08Yg8acAOCZtwUhU61g0EnlJ8Ck5Zks5sBU40gaJpZM4NxEC9> .

-- Yinoussa ADAGOLODJO Doctorant en Simulation Médicale Laboratoire ICube - UMR 7357 IRCAD Hôpitaux Universitaires 1 place de l'hôpital F. 67091 Strasbourg Cedex

damienmarchal · 2017-06-13T10:02:58Z

Hi @younesssss,
Any news on this PR ?

EDIT: In your PR you use @damien-marchal instead @damienmarchal... if you spell it with the '-' it is not me :)

guparan · 2017-07-06T16:08:29Z

[ci-build]
@matthieu-nesme could you update your review?

matthieu-nesme

inlined comments

to be coherent activate_projectVelocity should also be added to PartialFixedConstraint

matthieu-nesme · 2017-07-07T10:01:52Z

modules/SofaBoundaryCondition/FixedConstraint.inl

+    const SetIndexArray & indices = this->d_indices.getValue();
+    helper::WriteAccessor<DataVecDeriv> res ( mparams, vData );
+    helper::WriteAccessor<DataVecDeriv> resFree ( mparams, vData );
+


resFree must be removed (the same as res)

hugtalbot · 2017-07-24T18:34:44Z

please @younesssss finalise your PR

…traint component

adagolodjo · 2017-08-18T09:17:33Z

I don't understand this conflict because I have no conflict on my machine even when I took an update. I resolved then the conflict on github interface. Thanks

…traint component

guparan · 2017-08-23T12:51:11Z

Hi @younesssss, I just rebased your PR to remove the 5 merge commits. Let's [ci-build] now...

marques-bruno · 2017-08-25T12:41:48Z

@younesssss, Looks like the last modifs broke the code. could you investigate?

…indices in FEMGridBehaviorModel.inl

hugtalbot · 2017-09-01T15:40:05Z

Hey @younesssss only some small last fixes are to bring to your PR in order to compile with options. Just grep all codes depending FixedConstraint to make sure about your PR. You're almost there!

hugtalbot · 2017-09-01T17:20:00Z

[ci-build]

…nstraint dependances

…nstraint dependencies

hugtalbot · 2017-09-05T08:23:04Z

[ci-build]

guparan · 2017-09-13T12:38:16Z

@younesssss, your PR has been wip for a long time. Could you have a look at the build errors please?

guparan · 2017-10-12T09:40:43Z

Thanks for your fix @garciaguevara
[ci-build]

hugtalbot · 2017-10-18T15:52:10Z

[ci-build]

…velocity and free velocity

# Conflicts: # modules/SofaBoundaryCondition/FixedConstraint.h

…traint component

…indices in FEMGridBehaviorModel.inl

…nstraint dependencies

guparan · 2017-10-18T16:24:45Z

[ci-build]

hugtalbot · 2017-10-19T07:25:17Z

@matthieu-nesme we finally corrected the PR, I propose to merge it.
About the PartialFixedConstraint, it has been discussed at the STC that only one code should remain. The Partial- and FixedConstraint should be merged. I will work on it if this is fine to you.

matthieu-nesme suggested changes Jun 6, 2017

View reviewed changes

guparan changed the title ~~Fixe constraint features~~ [SofaBoundaryCondition] ADD flag PROJECTVELOCITY Jun 6, 2017

guparan added enhancement About a possible enhancement pr: status to review To notify reviewers to review this pull-request labels Jun 6, 2017

damienmarchal self-assigned this Jun 6, 2017

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 Jun 7, 2017

adagolodjo pushed a commit to mimesis-inria/sofa that referenced this pull request Jun 29, 2017

STYLE: rename variables as the convention sofa-framework#288

d52453a

matthieu-nesme suggested changes Jul 7, 2017

View reviewed changes

adagolodjo pushed a commit to mimesis-inria/sofa that referenced this pull request Jul 27, 2017

PR: sofa-framework#288 add d_projectVelocity data in PartialFixedCons…

6dca496

…traint component

marques-bruno 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 18, 2017

guparan force-pushed the FixeConstraint_Features branch from e32397c to 839cc0b Compare August 23, 2017 12:50

guparan pushed a commit to mimesis-inria/sofa that referenced this pull request Aug 23, 2017

STYLE: rename variables as the convention sofa-framework#288

6c58d66

guparan pushed a commit to mimesis-inria/sofa that referenced this pull request Aug 23, 2017

PR: sofa-framework#288 add d_projectVelocity data in PartialFixedCons…

77341bb

…traint component

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 Aug 29, 2017

adagolodjo pushed a commit to mimesis-inria/sofa that referenced this pull request Aug 31, 2017

PR: sofa-framework#288 change constraint->f_indices to constraint->d_…

a96b58d

…indices in FEMGridBehaviorModel.inl

adagolodjo pushed a commit to mimesis-inria/sofa that referenced this pull request Sep 5, 2017

PR: sofa-framework#288, change some f_indices to d_indices in FixedCo…

3ba6c3b

…nstraint dependances

adagolodjo pushed a commit to mimesis-inria/sofa that referenced this pull request Sep 5, 2017

PR: sofa-framework#288, change some f_indices to d_indices in FixedCo…

3e688a1

…nstraint dependencies

garciaguevara added a commit to mimesis-inria/sofa that referenced this pull request Oct 12, 2017

FIX sofa-framework#288 Rename data member variables

d867495

yinoussa2014 and others added 9 commits October 18, 2017 18:09

add flag PROJECTVELOCITY in FixedConstraint component to set to zero …

d15a4c0

…velocity and free velocity

projectVelocity use Data flag instead of #define Flag

b568e24

STYLE: rename variables as the convention sofa-framework#288

3122d02

# Conflicts: # modules/SofaBoundaryCondition/FixedConstraint.h

PR: sofa-framework#288 add d_projectVelocity data in PartialFixedCons…

04da9dd

…traint component

Fix indentation in Binding.cpp

d613d37

PR: sofa-framework#288 change constraint->f_indices to constraint->d_…

d9e2786

…indices in FEMGridBehaviorModel.inl

PR: sofa-framework#288, change some f_indices to d_indices in FixedCo…

5486c7f

…nstraint dependencies

FIX sofa-framework#288 Rename data member variables

b5650c0

FIX compilation in SceneColladaLoader and remove commented code

950117f

guparan force-pushed the FixeConstraint_Features branch from 4f5df4e to 950117f Compare October 18, 2017 16:23

hugtalbot 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 Oct 19, 2017

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 Oct 25, 2017

hugtalbot merged commit d45114f into sofa-framework:master Oct 30, 2017

untereiner deleted the FixeConstraint_Features branch October 30, 2017 20:03

guparan added this to the v17.12 milestone Dec 14, 2017

sofa-framework deleted a comment from adagolodjo Oct 14, 2018

sofa-framework deleted a comment from adagolodjo Oct 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SofaBoundaryCondition] ADD flag PROJECTVELOCITY #288

[SofaBoundaryCondition] ADD flag PROJECTVELOCITY #288

adagolodjo commented Jun 6, 2017 •

edited by guparan

Loading

sofabot commented Jun 6, 2017

matthieu-nesme left a comment

matthieu-nesme Jun 6, 2017

matthieu-nesme Jun 6, 2017

guparan commented Jun 6, 2017

damienmarchal commented Jun 6, 2017 •

edited

Loading

adagolodjo commented Jun 6, 2017 via email

damienmarchal commented Jun 13, 2017 •

edited

Loading

guparan commented Jul 6, 2017

matthieu-nesme left a comment

matthieu-nesme Jul 7, 2017

hugtalbot commented Jul 24, 2017

adagolodjo commented Aug 18, 2017

guparan commented Aug 23, 2017

marques-bruno commented Aug 25, 2017

hugtalbot commented Sep 1, 2017

hugtalbot commented Sep 1, 2017

hugtalbot commented Sep 5, 2017

guparan commented Sep 13, 2017

guparan commented Oct 12, 2017

hugtalbot commented Oct 18, 2017

guparan commented Oct 18, 2017

hugtalbot commented Oct 19, 2017

[SofaBoundaryCondition] ADD flag PROJECTVELOCITY #288

[SofaBoundaryCondition] ADD flag PROJECTVELOCITY #288

Conversation

adagolodjo commented Jun 6, 2017 • edited by guparan Loading

sofabot commented Jun 6, 2017

matthieu-nesme left a comment

Choose a reason for hiding this comment

matthieu-nesme Jun 6, 2017

Choose a reason for hiding this comment

matthieu-nesme Jun 6, 2017

Choose a reason for hiding this comment

guparan commented Jun 6, 2017

damienmarchal commented Jun 6, 2017 • edited Loading

adagolodjo commented Jun 6, 2017 via email

damienmarchal commented Jun 13, 2017 • edited Loading

guparan commented Jul 6, 2017

matthieu-nesme left a comment

Choose a reason for hiding this comment

matthieu-nesme Jul 7, 2017

Choose a reason for hiding this comment

hugtalbot commented Jul 24, 2017

adagolodjo commented Aug 18, 2017

guparan commented Aug 23, 2017

marques-bruno commented Aug 25, 2017

hugtalbot commented Sep 1, 2017

hugtalbot commented Sep 1, 2017

hugtalbot commented Sep 5, 2017

guparan commented Sep 13, 2017

guparan commented Oct 12, 2017

hugtalbot commented Oct 18, 2017

guparan commented Oct 18, 2017

hugtalbot commented Oct 19, 2017

adagolodjo commented Jun 6, 2017 •

edited by guparan

Loading

damienmarchal commented Jun 6, 2017 •

edited

Loading

damienmarchal commented Jun 13, 2017 •

edited

Loading