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

[SofaKernel] FIX in TetrahedronFEMForceField & TetrahedronSetTopologyAlgorithm #973

Merged
merged 6 commits into from
Apr 18, 2019

Conversation

damienmarchal
Copy link
Contributor

Hi, here is the content:

  1. [SofaKernel] FIX segfault in TetrahedronSetGeometryAlgorithms
    The segfault happens when the component is located in a node without
    a topology and a mechanical object. Then the 'draw' function is crashing.

  2. [SofaKernel] ADD a link to manually set the topology in TetrahedronFEMForceField
    The component was searching the topology in the context which was:

  • implicit
  • constraining the organization of components the scene graph.

To solve this I added a SingleLink that allows to specify the topology.
If the link is not set then the existing behavior is 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.

The segfault happens when the component is located in a node without
a topology and a mechanical object. Then the 'draw' function is crashing.
…MForceField

The component was searching the topology in the context which was:
- implicit
- constraining the organization of components the scene graph.

To solve this I added a SingleLink that allows to specify the topology.
If the link is not set then the existing behavior is used.
@damienmarchal damienmarchal added pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request labels Mar 28, 2019
@hugtalbot
Copy link
Contributor

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

@hugtalbot hugtalbot added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Apr 3, 2019
@epernod
Copy link
Contributor

epernod commented Apr 5, 2019

there are new tests failure:


FastTriangularBendingSprings.scn:0, Dynamic Parser, Priority: Normal
--
[EdgeSetGeometryAlgorithms(edgeSetGeometryAlgorithms1)] Unable to get a valid topology from the context

@epernod epernod added pr: status wip Development in the pull-request is still in progress and removed pr: status ready Approved a pull-request, ready to be squashed labels Apr 8, 2019
@damienmarchal
Copy link
Contributor Author

Thank for the reviewed.
I checked the example file and my thinking is that this example is wrong and my PR is legitimate.

  1. if you change in the example to turn visualization of the edge set it will crash.
  2. The edgeset is in the root node and so the existing code to retrieve the context in the init() is not working as we expect.
  3. The code in the PR is right at detecting and reporting an error when this case is detected.

So i moved the EdgeSet into the same node as the topology. So the error does not pop up.

@damienmarchal damienmarchal added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Apr 15, 2019
@epernod
Copy link
Contributor

epernod commented Apr 15, 2019

yes, a EdgeSetGeometryAlgorithms should not be inside a node without mechanicalObject nor a topology container (I know a lot of check are still missing :( ).
I'm even surprise, it works with this:

                <MeshTopology src="@loader" />
                <EdgeSetGeometryAlgorithms />

@epernod epernod added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Apr 17, 2019
@epernod epernod merged commit f1d7e29 into sofa-framework:master Apr 18, 2019
@guparan guparan added this to the v19.06 milestone Jun 20, 2019
@damienmarchal damienmarchal deleted the sofaMinorFix branch November 13, 2019 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix Fix a bug pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants