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] Major Change in Topology Containers #967

Merged
merged 30 commits into from Apr 18, 2019

Conversation

@epernod
Copy link
Contributor

commented Mar 14, 2019

  • Major change:
    Right now in Edge/Triangle/Quad/Tetrahedron/Hexahedron TopologyContainer, by default all cross element buffers like:
    EdgesInTriangles or TrianglesAroundEdges as well as d_triangles array inside a TetrahedronToplogy where empty at start and created only the first time a getter is called.
    For example: getEdgesInTriangleArray() was calling: if(m_edgesInTriangleArray.empty()) createEdgesInTriangleArray()

This works but means at start component are not fully init (potential unstable behavior) and there is no warranty at which timestep buffer will be created.
Furthermore means that getter are not const and could change the content of the container which is a bad design (in my opinion)

This PR force the computation of all buffers at component init using a dedicated method initTopology() which is called in cascade -> Tetrahedron::initTopology() will call Triangle::initTopology() -> Edge::initTopology()
and remove all buffer creation call from the getter.

  • Additional changes:
    • Add more check in the buffer creation methods. Potentially exit if creation is not possible (will return bool in future changes).
    • Always check buffer size access in the getter. If out of bound acces, returns Invalid element and only emit a warning if CHECK_TOPOLOGY is ON, as this call could be logical and the caller should check the invalid return.
    • Update all topologicalMapping to init the target topology at end of mapping init.
    • Add some static invalid buffers in sofa::core::topology::Topology.h -> Im not fully happy about those add. Any better suggestion is welcomed.

fixes #746
fixes #955
fixes #956


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.

epernod added some commits Mar 12, 2019

[SofaBaseTopology] Add initTopology methods in Edge/Triangle/Tetrahed…
…ron container to create all buffers at start. Called by component init method in cascade.
[SofaBaseTopology] Update all buffer creation method of Edge and Tria…
…ngle container to have more check and return if failed.
[SofaBaseTopology] Update all getter of Edge and Triangle container t…
…o only return good or invalid value and do not force buffer creation anymore.
[SofaBaseTopology] Update all getter and creation method from Tetrahe…
…dron container to have better check at creation and no creation in getter
[SofaBaseTopology] Add initTopology methods in Quad & Hexahedron cont…
…ainer to create all buffers at start. Called by component init method in cascade.
[SofaBaseTopology] Update all buffer creation method of Quad and Hexa…
…hedron container to have more check and return if failed.
[SofaBaseTopology] Update all getter of Quad and Hexahedron container…
… to only return good or invalid value and do not force buffer creation anymore.
@guparan

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

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

epernod and others added some commits Mar 26, 2019

[SofaBaseTopology] FIX: should not call create edgeAroundvertex buffe…
…r inside createEdge to avoid infinite loop.
[SofaKernel] FIX bug in MeshTopology
In issue 746 is reported a bug when trying trying to simulate
scenes while the underlying topology is containing empty data set.

I narrowed the problem to be in LocalMinDistance::computeIntersection.
In this function we access the trianglesAroundVertex from a MeshTopology.

with code like that:
```cpp
 if (m_edgesInTetrahedron.empty() || i > m_edgesInTetrahedron.size()-1)
         createEdgesInTetrahedronArray();
    return m_edgesInTetrahedron[i];
```

While if there is not "nothing" to create because there is no edges/triangles
it access in there is still a return of m_edgesInTetrahedron[i] with indice = 0

To avoid that I change the code for:
```cpp
  const MeshTopology::EdgesInTetrahedron& MeshTopology::getEdgesInTetrahedron(TetraID i)
 {
     if (m_edgesInTetrahedron.empty() || i > m_edgesInTetrahedron.size()-1)
         createEdgesInTetrahedronArray();
    return m_edgesInTetrahedron[i];

    if (i < m_edgesInTetrahedron.size())
        return m_edgesInTetrahedron[i];

    return InvalidEdgesInTetrahedron;
}
```

So if there is nothing created we returns a valid object indicating either an
empty or one filled with invalid values.

@epernod epernod referenced this pull request Apr 2, 2019

Closed

[SofaKernel] FIX issue 746 (bug in MeshTopology) #977

0 of 6 tasks complete
@damienmarchal

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

Sorry @epernod I didn't noticed that you looked at it. I was focused on the open issue.

epernod added some commits Apr 9, 2019

[SofaKernel] unify invalid topology buffers declaration, use invalidS…
…et from Topology.h instead of specific emptyArray from BaseMeshTopology and use invalid fixed_array specification from BaseMeshTopology.

@epernod epernod force-pushed the epernod:topology_changes_fix branch from 1cdcfd1 to 7a1d09f Apr 9, 2019

[SofaGeneralLoader] By default computeSubElements in GmshMeshLoader e…
…xcept if specified to not do so using Data<bool> d_createSubElements

epernod added some commits Apr 10, 2019

[SofaTopologyMapping] Fix Quad2TriangleTopologicalMapping, need to ad…
…d directly triangle to output container and not use the modifier as containers are not yet well init
[SofaTopologyMapping] Fix Hexa2QuadTopologicalMapping, need to add di…
…rectly quads to output container and not use the modifier as containers are not yet well init
@epernod

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Should fix issues: #746, #956 and probably #955

@damienmarchal

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

This is ready ???? So I merge it. :)

@damienmarchal damienmarchal merged commit 23a75b0 into sofa-framework:master Apr 18, 2019

7 checks passed

Dashboard Builds triggered.
Details
[with-regression-tests] Triggered in latest build.
Details
[with-scene-tests] Triggered in latest build.
Details
centos_clang-5_options Build OK. FIXME: 1 unit, 0 scene, 12 regressions
Details
mac_clang-3.5_options Build OK. FIXME: 2 units, 1 scene, 12 regressions
Details
ubuntu_gcc-5.4_options Build OK. FIXME: 0 unit, 0 scene, 11 regressions
Details
windows7_VS-2015_options_amd64 Build OK. FIXME: 2 units, 1 scene, 11 regressions
Details

@epernod epernod deleted the epernod:topology_changes_fix branch May 4, 2019

@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.