-
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
[SofaSimulationCore] Clean free motion animation loop #1930
Conversation
@@ -59,7 +57,7 @@ class SOFA_SOFACONSTRAINT_API FreeMotionAnimationLoop : public sofa::simulation: | |||
~FreeMotionAnimationLoop() override ; | |||
|
|||
sofa::core::behavior::ConstraintSolver *constraintSolver; | |||
component::constraintset::LCPConstraintSolver::SPtr defaultSolver; | |||
sofa::core::behavior::ConstraintSolver::SPtr defaultSolver; |
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.
We could add a description for the doxy doc:
///< pointers towards a possible ConstraintSolver present in the scene graph
///< pointers towards a default ConstraintSolver used in case none was found in the scene graph
[ci-build][with-all-tests] |
@@ -140,11 +140,12 @@ void DefaultAnimationLoop::step(const core::ExecParams* params, SReal dt) | |||
} | |||
sofa::helper::AdvancedTimer::stepEnd("UpdateMapping"); | |||
|
|||
if (!SOFA_NO_UPDATE_BBOX) | |||
#if SOFA_NO_UPDATE_BBOX == 0 |
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.
Hi alex,
If possible, the rules is to do as much as possible
if(SOFA_NO_UPDATE_BBOX)
{
....
}
The rational behind is that the conditional content is still compiled (eg: in the CI) so this ease maintenance and code refactoring while the compiler's optimizer can strip-out the if() when it is false.
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.
Since SOFA_NO_UPDATE_BBOX is a preprocessor variable, it makes more sense to me to use a preprocessor condition on it. IDEs would color the code differently on a deactivated code.
If you put the condition in a traditional condition, I am not sure the compiler compiles the code if the condition is false. IMO it sees it is false and does not bother compiling it. But I may be wrong, I don't know it for a fact.
The preprocessor condition would also remove the warning "the condition is always true".
A third alternative is
if constexpr (SOFA_NO_UPDATE_BBOX) { }
What do you think about this one?
Honestly, I don't have a strong preference. I don't mind reverting this change
@@ -286,11 +259,12 @@ void FreeMotionAnimationLoop::step(const sofa::core::ExecParams* params, SReal d | |||
} | |||
} | |||
|
|||
if (!SOFA_NO_UPDATE_BBOX) | |||
#if SOFA_NO_UPDATE_BBOX == 0 |
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.
So it seems to you are doing the inverse move from what I consider the best way to do...could you elaborate a bit why ?
[ci-build][with-all-tests] |
Looks fine to me and to the CI as well |
Minor cleaning:
sofa/SofaKernel/modules/SofaSimulationCore/src/sofa/simulation/UpdateBoundingBoxVisitor.cpp
Line 52 in 2072f96
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