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

[SofaBoundaryCondition] make LinearMovementConstraint accept absolute movements #394

Merged
merged 3 commits into from
Nov 8, 2017
Merged

[SofaBoundaryCondition] make LinearMovementConstraint accept absolute movements #394

merged 3 commits into from
Nov 8, 2017

Conversation

raphaeldeimel
Copy link

@raphaeldeimel raphaeldeimel commented Sep 7, 2017 •
edited by guparan
Loading

This is small feature addition.

Currently, LinearMovementConstraint only takes trajectories relative to the rest position of the MechanicalState. Sometimes though, the trajectory is specified in world frame coordinates, and especially with Rigid3D (i.e. orientations), conversion is not trivial for the user

This patch introduces a switch "relativeMovements" (default true).
When disabled, trajectories can be specified in world frame coordinates.

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
Copy link
Collaborator

sofabot commented Sep 7, 2017

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

@damienmarchal
Copy link
Contributor

damienmarchal commented Sep 8, 2017 •
edited
Loading

Hi Raphael,

Many thank for your pull request.
The feature sounds appealing and at first sight it looks ok. I have never used this component so if someone can give feedback it would be great.

EDIT: do you mind if I push some cleaning commit on this component in your PR. I would like to add some test cases for this component.

@guparan guparan added enhancement About a possible enhancement pr: status to review To notify reviewers to review this pull-request labels Sep 11, 2017
@guparan
Copy link
Contributor

guparan commented Sep 11, 2017

Hi @raphaeldeimel,
Thank you for your PR. I just rebased it to remove the empty commit with duplicated message.
[ci-build]

@guparan guparan changed the title make LinearMovementConstraint also accept absolute movements [SofaBoundaryCondition] make LinearMovementConstraint accept absolute movements Sep 11, 2017
@guparan
Copy link
Contributor

guparan commented Sep 13, 2017 •
edited
Loading

Shouldn't this code also be contained in a if (relativeMovements.getValue())?

@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 Sep 13, 2017
@guparan
Copy link
Contributor

guparan commented Sep 13, 2017 •
edited
Loading

Another remark from SOFA-dev meeting: relativeMovements should be false by default to keep current behavior.

@guparan
[SofaBoundaryCondition] CLEAN LinearMovementConstraint
5286bbc 
Remove commented cerr and rename Data relativeMovements to d_relativeMovements
@guparan
Copy link
Contributor

guparan commented Oct 23, 2017

I think the default value was misunderstood.
default to true = relative coordinates = current behavior = nothing changes by default = we are happy
[ci-build]

@guparan guparan 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 23, 2017
guparan
guparan reviewed Oct 25, 2017
View reviewed changes
modules/SofaBoundaryCondition/LinearMovementConstraint.inl
@@ -248,24 +249,15 @@ template <class MyCoord>
void LinearMovementConstraint<DataTypes>::interpolatePosition(Real cT, typename std::enable_if<!std::is_same<MyCoord, defaulttype::RigidCoord<3, Real> >::value, VecCoord>::type& x)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

From SOFA-dev meeting: shouldn't this function handle d_relativeMovements too?

Copy link
Contributor

Choose a reason for hiding this comment

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

ping @raphaeldeimel :-)

Copy link
Author

Choose a reason for hiding this comment

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

you're totally right ;)

@guparan
Copy link
Contributor

guparan commented Nov 6, 2017

@raphaeldeimel did you see my comment in LinearMovementConstraint.inl?

@guparan
Copy link
Contributor

guparan commented Nov 7, 2017

[ci-build]

@guparan 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 Nov 8, 2017
@guparan guparan merged commit a0bc024 into sofa-framework:master Nov 8, 2017
@guparan guparan added this to the v17.12 milestone Dec 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guparan guparan guparan left review comments

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
v17.12
Development

Successfully merging this pull request may close these issues.
4 participants
@raphaeldeimel @sofabot @damienmarchal @guparan