The scripted SlicerCIP “Calibration” module implements a reduced Slicer “Segment Editor” in its layout to define “Air” and “Blood” regions in lung CT.
The startup works fine, but a script “Reload” in development mode causes a crash as soon as paint drops are placed in the mentioned regions (Slicer 5.0.3 stable and Slicer 5.1).
I do not see the error, thank you for any ideas.
After a quick readthrough I also haven’t found the potential problem, but in case others don’t have time soon to join the conversation, one suggestion I’d have is to start commenting out parts of the code and narrow down the reason for the crash that way. I mean for example comment out all the segment editor related things and verify there is no crash. Fine, then you start adding back in the code piece by piece, and see which line reintroduces the crash. Maybe not a big help but this is what occurs to me now…
Thank you @cpinter - I tried that in many different combinations this afternoon but the crash only happens if I include this line .
Also tried to implement
if self.segmentEditorNode:
slicer.mrmlScene.RemoveNode(self.segmentEditorNode)
self.segmentEditorNode = None
if self.segmentEditorWidget:
self.segmentEditorWidget.setActiveEffect(None)
self.segmentEditorWidget = None
in the cleanup function.
I´m just maintaining the code but it would be great to understand and fix this as in the current state the module seems to be single-use only.
This line looks suspicious:
It is generally a bad idea to store node pointers as member variables. In modules, nodes should be kept track of using references: set node reference from parameter node when creating / selecting on UI, get as referenced node from parameter node when accessing. I know you didn’t implement the module but changing this could fix the crash.
Not sure I understand. The Reload button is supposed to be used while you develop the module, to update the code of the module so you can test it as you go with the implementation. What do you use it for?
Will try that, thank you.
absolutely, I am aware of that. Probably best to implement a “Reset” button to clear the segmentation.
But this will probably not solve the crash tendency during writing this portion of code, so it would be good to find the reason.
This is definitely incorrect:
self.segmentEditorNode = slicer.mrmlScene.AddNewNodeByClass("vtkMRMLSegmentEditorNode")
self.segmentEditorNode.SetSingletonTag("CIP_Calibration")
The problem is that after reload you’ll end up with having multiple singleton nodes in the scene with the same singleton tag, which may have unforeseeable consequences, even a crash. The correct usage would be to first try to get the singleton node from the scene and if it does not exist then add it, as it is done here:
Nevertheless, we should implement safeguards against the incorrect API use (e.g., prevent changing the singleton tag to a tag that already exists in the scene), and see if we can add some extra checks to prevent the crash.
Good catch @lassoan. This, in combination with not deleting the SingletonNode in cleanup(), solved it.
See a6f71e1
Thank you @rbumm, your fix looks good. I’ve pushed a commit with an additional null-pointer check in Slicer core that avoids this crash, even if the Slicer API is misused.
Nice, simpler than what I suggested. Still, I consider storing node pointers in member variables in Python a big risk, but a quick fix is better than no fix. Thanks @lassoan