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

[SofaBaseTopology] Fix SparseGridTopology initialization with an input mesh #670

Merged
merged 6 commits into from Aug 1, 2018

Conversation

@hugtalbot
Copy link
Contributor

hugtalbot commented May 29, 2018

Fixes #308 with initialization of SparseGridTopology with a mesh

Before, a 1% scaling was automatically applied on the grid bounding an input mesh. Removing the scaling was leading a crash since vertices could be exactly on the boundary surface (surface of the BBox of the mesh).
Now a safety check is done and a final check is done to alert if a vertex is not found.

Minor cleans of the msg API are also added.

@epernod @fredroy your feedback using your own scenes involving the SparseGrid would be useful.


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 added some commits May 29, 2018

FIX issue #308 with initialization of SparseGridTopology with a mesh
Before, a one-percent scaling was automatically applied on
the grid bounding an input mesh. The geometry was therefore scaled.

Clean the msg API

@hugtalbot hugtalbot self-assigned this May 29, 2018

@hugtalbot hugtalbot requested review from fredroy and epernod May 29, 2018

@hugtalbot hugtalbot changed the title Fix issue 308 sparsegrid Fix SparseGridTopology with a mesh (see issue 308) May 29, 2018

@hugtalbot hugtalbot changed the title Fix SparseGridTopology with a mesh (see issue 308) [SofaBaseTopology] Fix issue 308 sparsegrid May 29, 2018

@hugtalbot hugtalbot changed the title [SofaBaseTopology] Fix issue 308 sparsegrid [SofaBaseTopology] Fix SparseGridTopology initialization with an input mesh May 29, 2018

@hugtalbot

This comment has been minimized.

Copy link
Contributor Author

hugtalbot commented May 29, 2018

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

@@ -1419,8 +1426,8 @@ void SparseGridTopology::buildVirtualFinerLevels()
_virtualFinerLevels[0]->_fillWeighted.setValue( _fillWeighted.getValue() );
_virtualFinerLevels[0]->init();

sout<<"SparseGridTopology "<<getName()<<" buildVirtualFinerLevels : ";
sout<<"("<<newnx<<"x"<<newny<<"x"<<newnz<<") -> "<< _virtualFinerLevels[0]->getNbHexahedra() <<" elements , ";
msg_info()<<"SparseGridTopology "<<getName()<<" buildVirtualFinerLevels : " << msgendl;

This comment has been minimized.

@damienmarchal

damienmarchal May 30, 2018

Contributor

There is no needs for msgendl at he end of message it is only to separate lines within a message.

sout<<"SparseGridTopology "<<getName()<<" buildVirtualFinerLevels : ";
sout<<"("<<newnx<<"x"<<newny<<"x"<<newnz<<") -> "<< _virtualFinerLevels[0]->getNbHexahedra() <<" elements , ";
msg_info()<<"SparseGridTopology "<<getName()<<" buildVirtualFinerLevels : " << msgendl;
msg_info()<<"("<<newnx<<"x"<<newny<<"x"<<newnz<<") -> "<< _virtualFinerLevels[0]->getNbHexahedra() <<" elements , " << msgendl;

This comment has been minimized.

@damienmarchal

damienmarchal May 30, 2018

Contributor

It is better (more efficient, more clear) to have a single message with multiple lines in it instead of two message.

This comment has been minimized.

@damienmarchal

damienmarchal May 30, 2018

Contributor

Here is an example of one message without line breaks.

 msg_info() <<"("<<newnx<<"x"<<newny<<"x"<<newnz<<") ->   "<<_virtualFinerLevels[0]->getNbHexahedra() <<" elements , " 

<<"("<<_virtualFinerLevels[i]->getNx()<<"x"<<_virtualFinerLevels[i]->getNy()

<<"x"<<_virtualFinerLevels[i]->getNz()<<") -> "<< _virtualFinerLevels[i]->getNbHexahedra() <<" elements , "

If you want linebreaks into a message...add msgendl where you want the break.

This comment has been minimized.

@hugtalbot

hugtalbot Jun 7, 2018

Author Contributor

Does my latest commit do what you expected @damienmarchal ?

@hugtalbot

This comment has been minimized.

Copy link
Contributor Author

hugtalbot commented Jun 7, 2018

@epernod did you have some time to look at the issue?
maybe this default scaling of the sparseGrid was making sense ..

@@ -840,38 +832,45 @@ void SparseGridTopology::voxelizeTriangleMesh(helper::io::Mesh* mesh,
const helper::vector< Vector3 >& vertices = mesh->getVertices();
const size_t vertexSize = vertices.size();
helper::vector< int > verticesHexa(vertexSize);
helper::vector< bool > facetDone;
const Vector3 delta = (regularGrid->getDx() + regularGrid->getDy() + regularGrid->getDz()) / 2;

This comment has been minimized.

@epernod

epernod Jun 27, 2018

Contributor

Not sure about this Delta value, If I understand well. If one of the direction is very big and the other are small. You could miss some points in the "big" resolution direction in your test below.

This comment has been minimized.

@hugtalbot

hugtalbot Jul 12, 2018

Author Contributor

this could explain the crashing scene, but I don't get why actually. Why could I miss some points in the "big" resolution direction?

This comment has been minimized.

@epernod

epernod Jul 24, 2018

Contributor

not sure to remember exactly what mean Dx, Dy and Dz as they are 3D.
But in a general way in 1 dimension, if for example: Dx = 1000 and Dy = Dz = 10.
delta = 1020/2 = 510.
So when looking at next point in Dx direction, if the scale is ~1000, your delta of 510 is too small.

@epernod

This comment has been minimized.

Copy link
Contributor

epernod commented Jun 28, 2018

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

@epernod epernod added status: wip and removed status: ready labels Jun 28, 2018

@epernod

This comment has been minimized.

Copy link
Contributor

epernod commented Jun 28, 2018

@hugtalbot, the change seems to break 2 scene tests. Will you have time to check if they are false negative due to the BB change?

https://ci.inria.fr/sofa-ci-dev/job/sofa-framework/job/PR-670/CI_CONFIG=centos_clang-3.4,CI_PLUGINS=options,CI_TYPE=release/5/warnings3Result/

@hugtalbot

This comment has been minimized.

Copy link
Contributor Author

hugtalbot commented Jul 13, 2018

@erik did you notice my remark to yours ?

@epernod

This comment has been minimized.

Copy link
Contributor

epernod commented Jul 24, 2018

@hugtalbot this is not me above :)

@hugtalbot

This comment has been minimized.

Copy link
Contributor Author

hugtalbot commented Jul 30, 2018

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

@guparan guparan merged commit 192ac9e into sofa-framework:master Aug 1, 2018

6 checks passed

Jk2/Dashboard Builds triggered.
Details
Jk2/Scene tests Triggered in latest build.
Details
Jk2/centos_clang-3.4_options Build OK. FIXME: 1 unit tests, 0 scene tests
Details
Jk2/mac_clang-3.5_options Build OK. FIXME: 51 unit tests, 17 scene tests
Details
Jk2/ubuntu_gcc-5.4_options Build OK. FIXME: 0 unit tests, 1 scene tests
Details
Jk2/windows7_VS-2015_options_amd64 Build OK. FIXME: 1 unit tests, 0 scene tests
Details

@hugtalbot hugtalbot deleted the hugtalbot:fix_issue_308_sparsegrid branch Sep 21, 2018

@guparan guparan added this to the v18.12 milestone Oct 26, 2018

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