Skip to content
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] Change triangles orientation in tetrahedron #878

Merged
merged 2 commits into from Jan 25, 2019

Conversation

4 participants
@epernod
Copy link
Contributor

commented Jan 2, 2019

Right now for a tetrahedron: [P0, P1, P2, P3] (see picture below)
The 4 triangles saved in the tetrahedron are:

  • T0: [P1, P3, P2]
  • T1: [P2, P3, P0]
  • T2: [P3, P1, P0]
  • T3: [P0, P1, P2]

This means the 4 triangles are clockwised oriented and thus their normals are going inside the tetrahedron. If there is a special reason for that I couldn't find it in the doc.

image

As Gmsh nice ascii picture (from gmsh full doc) and the 2nd picture suggest. I changed to have counter-clockwise orientation so triangles on borders are by default well oriented to have normals going out.

Then, T0 being the 2D plan [u,v], T1 sharing vector u and then T2 and T3 to close the tetrahedron
So new triangles are:

  • T0: [P0, P2, P1]
  • T1: [P0, P1, P3]
  • T2: [P1, P2, P3]
  • T3: [P0, P3, P2]

image


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.

[SofaBaseTopology] Change the orientation of triangles created by tet…
…rahedron topology to have triangle normals directed outside of the tetrahedron.
@epernod

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2019

small change but might be breaking so I put it as independant PR.

@epernod

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2019

tetra2triangletopologicalmapping_00000003

examples/Components/Topology/Tetra2TriangleTopologicalMapping.scn normals display with the fix.

@epernod

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2019

[ci-build][with-all-tests]

@hugtalbot

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

@jnbrunet do you see any clue why orientations of triangles in tetrahedron are not checked and not oriented outside by default ? do you see any impact on physics ?
I only see the case of BC implementation ..

@jnbrunet

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

Oups sorry @hugtalbot , I didn't see the notification that you ping me in this thread.

I don't know why the triangles were oriented this way. For the breaking part, if someone was using them for BC or collisions, it will definitely break.

The next thing I can see is, you are not only changing the orientation of the triangle, but also the ordering of these triangles inside the tetrahedron. If I do TetrahedronSetTopologyContainer::getLocalTrianglesInTetrahedron (const TriangleID i), I no longer have the same triangle as before. That might also break something, although I can't find anything with a quick search in Sofa using it.

Finally, these indices are hardcoded in the cpp, and not accessible from the outside. So if someone is using them, it means he copy/pasted them in his source file. They will no longer match the one of the tetra container. This might have weird effects. It could be a good idea to make them available either directly from the header or inside a get function so that in the future, anyone using them will be in sync with the tetra container. They have getLocalTrianglesInTetrahedron for this, I'm a little bit tired !

I guess it is ok to go forward with this, but have this PR in mind if you got "my pressure FF doesn't work anymore, what are you guys doing????" on the forum :-)

@jnbrunet

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

Just a small side note: Tetra2TriangleTopologicalMapping seems to be inverting the triangles from the container, so you might want to remove the default swap since it is no longer required

@epernod

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

thx @jnbrunet for your answer.
Indeed there is a missmatch with the Tetra2TriangleTopologicalMapping but I removed that in another PR. In fact the mapping was inverting the triangles because their orientation in the tetra-container was not logical to have a well oriented surface.

Let see if it breaks something. At least people know on whom to shoot.

@hugtalbot

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

Ok, so we agree on merging guys @epernod @jnbrunet

[SofaBaseTopology_test] Update tetrahedronSetTopologyContainer tests …
…on triangles to match new triangle orientation
@epernod

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

now that I have fixed the tests on tetrahedron yes.

@guparan guparan merged commit bd08743 into sofa-framework:master Jan 25, 2019

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: 0 unit tests, 0 scene tests 10 regressions
Details
Jk2/mac_clang-3.5_options Build OK. FIXME: 1 unit tests, 4 scene tests 8 regressions
Details
Jk2/ubuntu_gcc-5.4_options Build OK. FIXME: 0 unit tests, 0 scene tests 9 regressions
Details
Jk2/windows7_VS-2015_options_amd64 Build OK. FIXME: 1 unit tests, 1 scene tests 9 regressions
Details

@guparan guparan added this to the v19.06 milestone Jun 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.