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.

Sign up for GitHub

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

Jump to bottom

[SofaConstraint] ADD Data to show constraint forces #840

Merged
merged 12 commits into from
Dec 13, 2018
Merged

[SofaConstraint] ADD Data to show constraint forces #840

merged 12 commits into from
Dec 13, 2018

Conversation

zhongkaidefrost
Copy link
Contributor

@zhongkaidefrost zhongkaidefrost commented Nov 23, 2018 •
edited by damienmarchal
Loading

from comment:
The Data constraintForces is used to expose the intensities of constraint forces in the simulation. We use the constraint forces as the approximation of real contact forces. We use this Data to provide contact information for the catheter insertion. The user can easily check the constraint forces from the GenericConstraint component interface.

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.

@guparan guparan changed the title Sofa add data to show constranit forces [SofaConstraint] ADD Data to show constraint forces Nov 23, 2018
@guparan guparan added enhancement About a possible enhancement pr: status to review To notify reviewers to review this pull-request labels Nov 23, 2018
guparan
guparan suggested changes Nov 23, 2018
View reviewed changes
modules/SofaConstraint/GenericConstraintSolver.h Outdated
@@ -130,6 +130,8 @@ class SOFA_CONSTRAINT_API GenericConstraintSolver : public ConstraintSolverImpl
Data<int> currentIterations; ///< OUTPUT: current number of constraint groups
Data<double> currentError; ///< OUTPUT: current error
Data<bool> reverseAccumulateOrder; ///< True to accumulate constraints from nodes in reversed order (can be necessary when using multi-mappings or interaction constraints not following the node hierarchy)
Data<helper::vector< double >> constraintForces;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use d_ convention when defining new Data.
See [N5] in https://github.com/sofa-framework/sofa/blob/master/GUIDELINES.md#naming

@guparan
Copy link
Contributor

guparan commented Nov 23, 2018

Congrats for your first PR @zhongkaidefrost ! 👏
I just suggested a change, please check it when you have a few minutes :-)

@guparan 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 Nov 28, 2018
@epernod
Copy link
Contributor

epernod commented Nov 28, 2018

@zhongkaidefrost thanks for your PR.
I'm not sure to understand what the Data constraintForces is used for as it is filled but never used.
Could you explain in the description of your PR why you introduced this new Data.

In the same idea, it would be nice if you could add an inline doxygen comment to your Data.

@zhongkaidefrost
Copy link
Contributor Author

zhongkaidefrost commented Nov 30, 2018 •
edited
Loading

The Data constraintForces is used to provide the intensities of constraint forces in the simulation. We use the constraint forces as the approximation of real contact forces. We use this Data to provide contact information for the catheter insertion. The user can easily check the constraint forces from the GenericConstraint component interface.

@damienmarchal damienmarchal 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 30, 2018
@guparan 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 Dec 5, 2018
@damienmarchal
Copy link
Contributor

[ci-build] (because of weird CI-failure)

@damienmarchal damienmarchal 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 Dec 11, 2018
@epernod
Copy link
Contributor

epernod commented Dec 11, 2018

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

@guparan
Copy link
Contributor

guparan commented Dec 12, 2018

@damienmarchal Do you still plan to change the variable name?

@damienmarchal
Copy link
Contributor

damienmarchal commented Dec 12, 2018 •
edited
Loading

I forgot :)

EDIT: Done

@damienmarchal
[SofaConstraint] Rename Data field d_constraintForceEnable into d_com…
fddceda 
…puteConstraintForces

So the two related data field matches.
@damienmarchal
Copy link
Contributor

@guparan ready/merge ?

@damienmarchal damienmarchal 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 Dec 13, 2018
@damienmarchal damienmarchal merged commit d3870fc into sofa-framework:master Dec 13, 2018
@damienmarchal damienmarchal deleted the sofAddForce branch December 17, 2018 10:55
@guparan guparan added this to the v18.12 milestone Dec 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guparan guparan guparan approved these changes

@epernod epernod epernod approved these changes

Assignees
No one assigned
Labels
enhancement About a possible enhancement pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Milestone
v18.12
Development

Successfully merging this pull request may close these issues.
4 participants
@zhongkaidefrost @guparan @epernod @damienmarchal