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

[all] Update issofa bugfix #756

Merged
merged 28 commits into from Dec 19, 2018

Conversation

4 participants
@hugtalbot
Copy link
Contributor

hugtalbot commented Aug 27, 2018

Manual merge to close PR #218
Only one commit was not integrated: (MechanicalMatrixVisitor and MechanicalOperations) fix if using a Linear Solver, projective constraints were wrongly applied when a force field is in a child node

-- Notes:

This PR will be further discussed during STC#3.

issofa_bugfix

Cleans

  • BaseObject: Output message in serr for required datas. Error word should be reserved for messages that will make the application fail.
  • RayTriangleVisitor and SceneLoaderFactory: clean warnings
  • ParticlesRepulsionForceField: Add empty implementation of addKToMatrix to get rid of console warnings.

Bugfixes

  • AttachConstraint: fix crash when indices are resized
  • BTDLinearSolver: fix constant log + operator new [] vs operator delete mismatch
  • CatmullRomSplineMapping: fix applyJ compilation
  • ConstantForceField: fix avoid crashes in draw when trying to apply a constantforcefield to a non existing point
  • DistanceGrid: fix incorrect use of serr
  • GeneralConstraintSolver: fix incorrect parent class in SOFA_CLASS macro
  • IndicesFromValues: fix use getTemplateName to avoid problems of links with other data
  • EulerImplicitSolver: fix Disable calls to solveConstraint from EulerImplicitSolver by default as it is not needed unless an very specific solver is added and it currently crashes in other cases + wrong argument order in calls to AdvancedTimer::stepNext()
  • FixedConstraint and PartialFixedConstraint: fix "fixed FixedConstraint & PartialFixedConstraint for size-varying mechanical objects".
  • MechanicalObject: fix crash in debug with null pointer
  • Mass: fix remove error logging in Mass method that are inherited from Forcefield API and that are not relevant for Mass
  • Mesh2PointMechanicalMapping: fix constraints ApplyJT
  • Metis(gk_arch.h): fix compilation with Visual Studio 2015
  • ParticlesRepulsionForceField and RepulsiveSpringForceField: fix avoid division by 0 on repulsion force fields
  • PersistentContactBarycentricMapping: fix init variables in constructor
  • RestShapeSpringsForceField: fix Runtime stiffness tunning was not working + optimisation and cleaning
  • SkinningMapping: fix compilation of SofaRigid
  • SofaViewer: fix crashes when current camera of pick-handler is NULL
  • SurfacePressureForceField: fix volume computation formula
  • TaitSurfacePressureForceField: fix contribution to the stiffness matrix for the second component df = P+dN in TaitSurfacePressureForceField
  • TopologicalMapping: fix display error messages when a TopologicalMapping failed to be created
  • TriangularFEMForceFieldOptim: fix principal values ordering with input matrix is diagonal + uninitialized value warning
  • VisualModelImpl: fix wrong object (triangles) called when adding quads in handleTopologyChange()
  • VoxelGridLoader: std::unique result was not used

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.

@hugtalbot

This comment has been minimized.

Copy link
Contributor Author

hugtalbot commented Aug 27, 2018

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

hugtalbot added some commits Sep 21, 2018

@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Nov 27, 2018

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

if (v.isNull())
{
msg_error() << "Accessing null VecDeriv";
}

This comment has been minimized.

@guparan

guparan Nov 28, 2018

Member

What should happen to v if v.isNull() ? This block does not pass in scene tests.

This comment has been minimized.

@epernod

epernod Nov 29, 2018

Contributor

I tried to understand but...
So when v.isNull, this means that v.index == 0.

So in the next lines, it enters this block for the first time and create a vector.

if (vectorsDeriv[v.index] == NULL)
{
        vectorsDeriv[v.index] = new Data< VecDeriv >;
        vectorsDeriv[v.index]->setName(v.getName());
        vectorsDeriv[v.index]->setGroup("Vector");

For example In the scene: examples/Components/collision/RuleBasedContactManager.scn this error happened when constraints are created here:

template<class DataTypes>
void PairInteractionConstraint<DataTypes>::storeLambda(const ConstraintParams* cParams, MultiVecDerivId res, const sofa::defaulttype::BaseVector* lambda)
{
    if (cParams)
    {
        storeLambda(cParams, *res[mstate1.get(cParams)].write(), *res[mstate2.get(cParams)].write(), *cParams->readJ(mstate1), *cParams->readJ(mstate2), lambda);
    }
}

but the scene seems to act normally... (it moves)

This comment has been minimized.

@epernod

epernod Nov 29, 2018

Contributor

I find it logical that a new Vecid is created when contact happened for the first time, but I don't understand why index is null.

@epernod

This comment has been minimized.

Copy link
Contributor

epernod commented Nov 28, 2018

[ci-build][with-regression-tests] 👍

if (v.isNull())
{
msg_error() << "Accessing null VecDeriv";
}

This comment has been minimized.

@epernod

epernod Nov 29, 2018

Contributor

I tried to understand but...
So when v.isNull, this means that v.index == 0.

So in the next lines, it enters this block for the first time and create a vector.

if (vectorsDeriv[v.index] == NULL)
{
        vectorsDeriv[v.index] = new Data< VecDeriv >;
        vectorsDeriv[v.index]->setName(v.getName());
        vectorsDeriv[v.index]->setGroup("Vector");

For example In the scene: examples/Components/collision/RuleBasedContactManager.scn this error happened when constraints are created here:

template<class DataTypes>
void PairInteractionConstraint<DataTypes>::storeLambda(const ConstraintParams* cParams, MultiVecDerivId res, const sofa::defaulttype::BaseVector* lambda)
{
    if (cParams)
    {
        storeLambda(cParams, *res[mstate1.get(cParams)].write(), *res[mstate2.get(cParams)].write(), *cParams->readJ(mstate1), *cParams->readJ(mstate2), lambda);
    }
}

but the scene seems to act normally... (it moves)

if (v.isNull())
{
msg_error() << "Accessing null VecDeriv";
}

This comment has been minimized.

@epernod

epernod Nov 29, 2018

Contributor

I find it logical that a new Vecid is created when contact happened for the first time, but I don't understand why index is null.

hugtalbot added some commits Dec 13, 2018

Remove change with checks on pointers in MechanicalObject in order to…
… keep the branch as a cleaning branch ionly
@hugtalbot

This comment has been minimized.

Copy link
Contributor Author

hugtalbot commented Dec 13, 2018

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

@hugtalbot

This comment has been minimized.

Copy link
Contributor Author

hugtalbot commented Dec 14, 2018

@epernod @guparan I did the change by removing the extra changes in MechanicalObject
Let me know whether this suits you

@hugtalbot

This comment has been minimized.

Copy link
Contributor Author

hugtalbot commented Dec 17, 2018

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

@epernod

This comment has been minimized.

Copy link
Contributor

epernod commented Dec 17, 2018

ok could you open an issue regarding the null vector to keep track of this bug. (maybe just a copy past of the lines)
And my comments regarding the investigation. You could even assign it to me! hurry up before I change my mind.

@@ -127,7 +127,7 @@ sofa::defaulttype::BaseMatrix* Mapping<In,Out>::createMappedMatrix(const behavio
sofa::defaulttype::BaseMatrix* result;
if( !this->areMatricesMapped() )
{
sout << "Mapping::createMappedMatrix() this mapping do not support matrices building. Set mapMatrices to true" << getClassName() << sendl;
msg_warning() << "Mapping::createMappedMatrix() this mapping do not support matrices building. Set mapMatrices to true" << getClassName();

This comment has been minimized.

@damienmarchal

damienmarchal Dec 18, 2018

Contributor

To be super pinchy (merge it witjout taking that into account)...
There is no need to add "Mapping::createMappedMatrix()" to the message as the emitting point is already in the msg_warning() macro. (additionally it make no sense to send to the user any details about c++ stuff)

}
if (err)
{
serr << "Error in " << __FUNCTION__ << " : wrong source hexa / destination point association" << sendl;

This comment has been minimized.

@damienmarchal

damienmarchal Dec 18, 2018

Contributor

miaouu

}
if (err)
{
serr << "Error in " << __FUNCTION__ << " : wrong source tetra / destination point association" << sendl;

This comment has been minimized.

@damienmarchal

damienmarchal Dec 18, 2018

Contributor

mheuu

}
if (err)
{
serr << "Error in " << __FUNCTION__ << " : wrong source quad / destination point association" << sendl;

This comment has been minimized.

@damienmarchal

damienmarchal Dec 18, 2018

Contributor

coincoin

}
if (err)
{
serr << "Error in " << __FUNCTION__ << " : wrong source triangle / destination point association" << sendl;

This comment has been minimized.

@damienmarchal

damienmarchal Dec 18, 2018

Contributor

msg_warning..

@damienmarchal damienmarchal merged commit 507c088 into sofa-framework:master Dec 19, 2018

7 checks passed

Jk2/Dashboard Builds triggered.
Details
Jk2/[with-regression-tests] Triggered in latest build.
Details
Jk2/[with-scene-tests] Triggered in latest build.
Details
Jk2/centos_clang-5_options Build OK. FIXME: 1 unit tests, 0 scene tests 4 regressions
Details
Jk2/mac_clang-3.5_options Build OK. FIXME: 1 unit tests, 4 scene tests 4 regressions
Details
Jk2/ubuntu_gcc-5.4_options Build OK. FIXME: 0 unit tests, 0 scene tests 4 regressions
Details
Jk2/windows7_VS-2015_options_amd64 Build OK. FIXME: 1 unit tests, 1 scene tests 4 regressions
Details
@damienmarchal

This comment has been minimized.

Copy link
Contributor

damienmarchal commented Dec 19, 2018

Ok, zouh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment