sofa icon indicating copy to clipboard operation
sofa copied to clipboard

[Topology] Fix topologyHandler removal

Open epernod opened this issue 2 years ago • 1 comments

A bug has been detected when creating and deleting component using TopologyHandler during the simulation.

Current Status

Right now in the code, components like FEM, Constraints, etc... are creating TopologyData and at init stage, those TopologyData are creating a TopologyHandler to support TopologicalChanges (only if the component is linked with a dynamic topology container). The actual design is:

  1. Create the TopologyHandler at component init (internally through the TopologyData creation).
  2. The TopologyHandler register itselft into the TopologyContainer to be able to receive topological events.
  3. When the simulation graph is destroyed, TopologyHandler is deleted and TopologyContainer is cleared.

Limitation/Bug

Scenario like using the mouse picking is creating a temporary Node with springFF or attachedConstraint component which is creating a TopologyHandler. When the mouse is released the Node is destroyed, deleting the components and the TopologyHandler as well. But the TopologyHandler is not properly removed from the dynamic topologyContainer list of TopologyHandler.

This is why in issue #3370 the scene crash as soon as a vertex is removed because the TopologyContainer is sending the POINTREMOVED event to all topologyHandler registered. Including the deleted one from the mouse attached constraint. The pointer to the topologyHandler is not null but pointed to a deleted object.

PR changes description

This PR and the PR #3271 (included in this on) are introducing a mechanism to better register and unregister a TopologyHandler from the TopologyContainer.

  1. register is done at creation of the TopologyHandler
  2. If TopologyHandler has well been added to the list manged by the Container, a internal bool m_isRegisterd is set to true.
  3. When the TopologyHandler is deleted (meaning its component owner is deleted). If m_isRegisterd == true, the TopologyHandler will unregister itself.
  4. When the TopologyContainer is deleted, it will go through each TopologyHandler it is managing and turn m_isRegisted to false. And then clear the list without deleting the Handler.

The bool m_isRegisterd is used because to unregister itself, the handler need to access the topologyContainer which could have already been destroyed (depending on the graph order). But the TopologyHandler doesn't now the container is null. That's why if the TopologyContainer is deleted first it sets all its topologyHander as unregistered.

Linked issues

Based on PR #3271 Fixes #3370 #3202


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).

epernod avatar Oct 08 '22 15:10 epernod

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

epernod avatar Oct 08 '22 20:10 epernod

Unit tests are failing, but are these maybe fixed in previous PR? @epernod

hugtalbot avatar Oct 18 '22 18:10 hugtalbot

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

fredroy avatar Oct 19 '22 04:10 fredroy

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

epernod avatar Oct 20 '22 12:10 epernod

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

fredroy avatar Oct 26 '22 00:10 fredroy