-
Notifications
You must be signed in to change notification settings - Fork 311
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.
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
[Simulation.Core] Refactor DefaultAnimationLoop + multithreading #3959
Conversation
[ci-build][with-all-tests] |
Sofa/framework/Simulation/Core/src/sofa/simulation/DefaultAnimationLoop.cpp
Show resolved
Hide resolved
Sofa/framework/Simulation/Core/src/sofa/simulation/DefaultAnimationLoop.cpp
Show resolved
Hide resolved
[ci-build][with-all-tests] |
1 similar comment
[ci-build][with-all-tests] |
Another benchmark: With parallelism
Without parallelism:
|
[ci-build][with-all-tests][force-full-build] |
Sofa/framework/Simulation/Core/src/sofa/simulation/DefaultAnimationLoop.cpp
Outdated
Show resolved
Hide resolved
examples/Component/SolidMechanics/FEM/TetrahedronFEMForceField.scn
Outdated
Show resolved
Hide resolved
examples/Component/SolidMechanics/FEM/TetrahedronFEMForceField.scn
Outdated
Show resolved
Hide resolved
[ci-build][with-all-tests] (check for MacOS) |
[ci-build][with-all-tests][force-full-build] |
void DefaultAnimationLoop::buildConstraintMatrix(core::ConstraintParams cparams) const | ||
{ | ||
sofa::helper::ScopedAdvancedTimer timer("buildConstraintMatrix"); | ||
unsigned int constraintId = 0; | ||
mechanicalvisitor::MechanicalBuildConstraintMatrix buildConstraintMatrix(&cparams, core::MatrixDerivId::constraintJacobian(), constraintId ); | ||
buildConstraintMatrix.execute(gnode); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function in a DefaultAnimationLoop does not make sense since only FF-based (weak) constraints can be handled with the DefaultAnimationLoop
What about removing it @alxbilger ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what it is supposed to do. I guess we can try to remove it and see if CI complains
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you tested it @alxbilger ?
I don't understand why the dashboard says 2 scenes are failing. When I see the result, I see only one |
CI sees apparently twice an error in advancedtimer .. strange
|
The logic was difficult to read in AnimateVisitor. Instead, all the steps of the animation loop are now explicit in the the code of the animation loop.
Co-authored-by: Hugo <hugo.talbot@sofa-framework.org>
There is an error in the test because timer ids changed in this PR |
794ec14
to
5653f87
Compare
[ci-depends-on] detected during build #13. To unlock the merge button, you must
|
# Conflicts: # Sofa/framework/Simulation/Core/src/sofa/simulation/DefaultAnimationLoop.cpp # Sofa/framework/Simulation/Core/src/sofa/simulation/DefaultAnimationLoop.h
[ci-depends-on] detected during build #14. To unlock the merge button, you must
|
The crash that happened on Windows also happens on the master branch on my computer. But I don't understand how it is not detected by the CI |
Maybe it is random, it does not crash on mine (but I only tried like 2, 3 times) on msvc |
It seems strange that the factorization of the mapping propagation at the end of the time step makes the scene crashes.. |
[ci-build][with-all-tests][force-full-build] |
[ci-depends-on] detected during build #15. To unlock the merge button, you must
|
[ci-build][with-all-tests][force-full-build] |
[ci-depends-on] detected during build #16. All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! 👍 |
Split all pipeline steps into smaller functions
Each step of the animation loop now corresponds to a function. For example, the following code:
is now in a function:
In the end, the pipeline is easier to read:
AnimateVisitor
AnimateVisitor
is no longer used. It was difficult to read its logic (the visitor called other visitors, it relies heavily on pruning which is easy to miss). Instead, I suggest to explicit all the steps from theAnimateVisitor
into theDefaultAnimationLoop
. This is closer to what is done in theFreeMotionAnimationLoop
. Plus, we can find common steps, which makes the code more consistent.Note also the introduction of
ComputeIsolatedForceVisitor
(see the comments in the code to understand its purpose).Multithreading
Since
AnimateVisitor
is no longer used,SolveVisitor
is used instead, providing optional multithreading, i.e. solving each ODE in parallel. Similar to what was done in theFreeMotionAnimationLoop
.Benchmark
Measured on
examples/Component/SolidMechanics/FEM/TetrahedronFEMForceField.scn
Without parallelism
With parallelism
The simulation is 2x faster. But there are 4 different objects, so we expected a speed-up of 4x. However, the speed-up is only about the
solve
function. Another time consuming function is theend event
when computing the von Misses stresses. Still, the solve function only shows a speedup of 2.68x, instead of 4x. I cannot explain it for now...[ci-depends-on https://github.com/sofa-framework/SofaPython3/pull/362]
By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).
Reviewers will merge this pull-request only if