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

ENH: Enable reset simulations #23

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Quilliams85
Copy link

Added ui button and logic to reset the deformations on a model from a simulation for the soft-tissue-simulation module. Also fixed the time step counter so that it updates the UI each step.

@RafaelPalomar RafaelPalomar self-assigned this Jul 23, 2024
Copy link
Collaborator

@RafaelPalomar RafaelPalomar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Quilliams85, thanks for the contribution. In general, this goes in the right direction. I think we can improve the re-usability of the code and its generalization. For that I proposed a couple of changes.

if self._sceneUp is False or self._simulationRunning is False:
slicer.util.errorDisplay("Can't advance the simulation forward. Simulation is not running.")
return

parameterNode.modelNode.GetUnstructuredGrid()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed, we should remove it

Comment on lines +96 to +98
def resetSimulation(self, parameterNode=None) -> None:
pass

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function has little use as is. I would move here all the lines involving member variables of this class from https://github.com/Quilliams85/Slicer-SOFA/blob/7bbc2cea19222da2e40646b99230668d78d0bfa9/SoftTissueSimulation/SoftTissueSimulation.py#L406-L417

self._dt = 0
self.initialgrid = []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I would leave this to the derived classes. Two reasons:

  • Not all simulations necessarily will deal with unstructured grids (some may use surface meshes as polydata)
  • Some simulations may imply the use and update of more than one model.

Comment on lines +410 to +413
points_vtk = numpy_to_vtk(num_array=self.initialgrid, deep=True, array_type=vtk.VTK_FLOAT)
vtk_points = vtk.vtkPoints()
vtk_points.SetData(points_vtk)
self.getParameterNode().modelNode.GetUnstructuredGrid().SetPoints(vtk_points)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pieper recommended to not to use this way of updating the mesh as the ownership of the underlying memory by sofa objects and vtk objects can cause this to crash. Instead, you can look at https://github.com/RafaelPalomar/SlicerSOFA/blob/ce4b4131a9ed4a0403641de4cf44efcf455d2c34/MultiMaterialSimulation/MultiMaterialSimulation.py#L289-L298 where I use the approach suggested by @pieper

As mentioned above, moving as much as possible to the base class will help reusing the code

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

Successfully merging this pull request may close these issues.

2 participants