sofa-framework / sofa

Real-time multi-physics simulation with an emphasis on medical simulation.
https://www.sofa-framework.org
GNU Lesser General Public License v2.1
871 stars 297 forks source link

[SofaCore SofaConstraint] Update FreeMotionAnimationLoop so that it can compute a linearised version of the constraint force. #459

Closed fjourdes closed 5 years ago

fjourdes commented 6 years ago

This PR diffs against the sofa-framework::issofa_constrainsolving branch until it has been merged into master.

Objectives

Requirements

The following PR are required in order to build this

For testing:

Changelog

API Breaking

[SofaCore]

FIX

[SofaConstraint]

UPDATES

[SofaSimulationCore]

[SofaBaseMechanics]

[SofaMeshCollision]

[SofaConstraint]

[SofaMiscMapping]

[SofaGeneralAnimationLoop]

ADD

[SofaConstraint]

[Examples]

Remarks


This PR:

Reviewers will merge only if all these checks are true.

fjourdes commented 6 years ago

I believe that if that branch makes it way to master, someone at mimesis should have close look too.

guparan commented 6 years ago

I am ready to rebase your branch on issofa_constraintsolving (updated with master). Are you OK for me to force push?

ChristianDuriez commented 6 years ago

You will do it in a branch of Master ? I was suppose to push it on sofaDefrost but if you are doing it on the Master, that's even better...

fjourdes commented 6 years ago

@guparan : I think it is fine to rebase with the updated issofa_constraint_solving branch. I believe the conflicts come from the override keyword that was added in many places in the code recently. The integration branch fjourdes:insimo_freemotion_integration will still work if someone wants want to try.

guparan commented 6 years ago

It's done. Yes most of the conflicts went from override keywords. @ChristianDuriez I did it on this branch to be able to easily rebase it on master when #484 will be merged.

hugtalbot commented 6 years ago

This PR will be rebased and reviewed this week. Any help in review would be most welcome @EulalieCoevoet ;)

guparan commented 6 years ago

This PR is now rebased and aiming master. Review shall begin!

damienmarchal commented 6 years ago

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

guparan commented 6 years ago

ping @EulalieCoevoet :-)

guparan commented 6 years ago

[ci-build]

hugtalbot commented 6 years ago

hey @EulalieCoevoet will you be available to review this PR by next week ? This PR starts being a bit old. Thanks !

EulalieCoevoet commented 6 years ago

Hi, Big sorry for the late answer... I'm looking at it now.

EulalieCoevoet commented 6 years ago

So I have tested the branch fjourdes:insimo_freemotion_integration with our plugins. Our tests and examples ran fine. I have read the very well detailed changelog (thanks for that) and from what I know it looks fine. I didn't review the files changes, because there is too much to look at... but I would agree to merge. Again, I'm really sorry for the late answer.

hugtalbot commented 6 years ago

Thank you for your feedback Eulalie! I guess some fixes / merges are required to fit the current master.

To start with, let's [ci-build]

guparan commented 6 years ago

@fjourdes could you please confirm that my fix in 792d95c is not breaking your changes?

guparan commented 6 years ago

Wrong fix replaced with correct one :-)

hugtalbot commented 6 years ago

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

hugtalbot commented 6 years ago

Damned, it seems there's some test failing with BilateralInteractionConstraint_test/0.checkVec3ConstrainedPositions

hugtalbot commented 6 years ago

@EulalieCoevoet there is an issue with the test, do you have time within the 7 upcoming days to have a look at it ? either the test needs to be updated, or the code to be fixed. Let us know if you don't ;)

hugtalbot commented 6 years ago

Ok, this PR is almost there. Only one test is failing. @fjourdes have you few minutes to check it out ? thx

hugtalbot commented 6 years ago

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

hugtalbot commented 6 years ago

test BilateralInteractionConstraint_test failing due to the constraint which is not exactly met

ChristianDuriez commented 6 years ago

Hi, I think I have solved the test problem.. by changing the test ! It was not designed to take into account the tolerance of the solver... see commit: https://github.com/fjourdes/sofa/commit/78b84592651836809078c18971300f6abde6806a

hugtalbot commented 6 years ago

Hi Christian,

Using a tolerance is working, but is this normal that the constraint is not exactly applied? (although it was before)

damienmarchal commented 6 years ago

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

damienmarchal commented 6 years ago

@hugtalbot , @ChristianDuriez Can you reproduce locally the problem that happens in ubuntu or is this a CI issue ?

ChristianDuriez commented 6 years ago

I am not on Ubuntu. It seems to be a problem with a test in the Flexible plugin.

hugtalbot commented 6 years ago

@ChristianDuriez One scene was crashing in SensableEmulation plugin : testOmniDriverEmu.scn I fixed it by adding the option : solveVelocityConstraintFirst="true". I have no idea why actually! Could you give some insight?

I fixed some added warnings, let me know if this was normal: Note that :

What appears really necessary, is to have some documentation on this constraint pipeline. This could be a good task for the STC#5 don't you think?

hugtalbot commented 6 years ago

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

hugtalbot commented 5 years ago

Conflicts are again fixed, if it compiles the PR will have to be merged asap

hugtalbot commented 5 years ago

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

damienmarchal commented 5 years ago

Merge ...

fangzhouzisb commented 4 years ago

Delay problem still exists using Geomagic plugin. See https://www.sofa-framework.org/community/forum/topic/problem-about-geomagic-plugin-in-v18-12/

hugtalbot commented 4 years ago

Thanks @fangzhouzisb This is not the correct place to ask, here the pull-request has been merged and focuses on FreeAnimationLopp Do not hesitate to create a dedicated issue referring (as you did) the forum topic.