Subject hierarchy listening to events

Development
1

Hi all,

I am working a module that relies heavily on the subject hierarchy. I need to listen to its modified/added and removed item events. Here is an abstract of what I do:

class MyClass(VTKObservationMixin):
  def __init__(self):
    VTKObservationMixin.__init__(self)
    shNode = slicer.vtkMRMLSubjectHierarchyNode.GetSubjectHierarchyNode(slicer.mrmlScene)
    self.addObserver(shNode, shNode.SubjectHierarchyItemModifiedEvent, self.shItemModifiedEvent)
  
  @vtk.calldata_type(vtk.VTK_LONG)
  def shItemModifiedEvent(self, caller, eventId, callData=None):
    print("SH Node mod")
    print("SH item ID: {0}".format(callData))

obj = MyClass()

shNode = slicer.vtkMRMLSubjectHierarchyNode.GetSubjectHierarchyNode(slicer.mrmlScene)

itemID = shNode.CreateFolderItem(shNode.GetSceneItemID(), 'Test')

wasModifying = shNode.StartModify()
shNode.SetItemName(itemID, 'NewName')
shNode.SetItemAttribute(itemID, 'NewAttribute', 'a')
shNode.EndModify(wasModifying)

This crashes on Slicer that I built manually.
The issues comes from theEndModify() that will re-invoke all the custom events. Those events however are invoked without a calldata and that makes the application crash when it tries to convert the calldata to long.

I am not sure what can be done for this. The naive solution I see would be to never use the calldata to answer the callback of the subject hierarchy. But then we would need a method on the subject hierarchy node to know what was the item ID that was last modified ?
Or should the VTK python command handle the case where the calldata is NULL better ?

Thanks !
Johan

2

@lassoan, @cpinter:
I am thinking to implement a method that stores the ID of the last modified item so it’s always accessible through python. Since you implemented this node, I was wondering what you were thinking ?

3

After talking with @lassoan, the current mechanism is generic and can handle any event. As you first suggested, we should instead fix the VTK wrapping layer to that the call data is None if the pointer is null.

From the application perspective, this would mean that we can’t know the specific ID or name or … that was modified and the UI should be refreshed.

4

For all types, the code could then be changed from:

PyObject* callDataAsInt = PyLong_FromLong(*reinterpret_cast<long*>(callData));
arglist = BuildCallDataArgList(obj2, eventname, callDataAsInt);

to

PyObject* callDataAsInt = callData != nullptr ? PyLong_FromLong(*reinterpret_cast<long*>(callData)) : nullptr;
arglist = BuildCallDataArgList(obj2, eventname, callDataAsInt);

(or the more explicit multi-line version …)

5

That makes sense to me. I will create a PR.
Thanks !

1 Like
6

Thanks @jcfr and @lassoan for helping Johan! I just managed to dig through the administrative stuff and get back to real work after I got back from my long trip.

7

I created a PR on VTK (https://gitlab.kitware.com/vtk/vtk/merge_requests/4513) and Slicer: https://github.com/Slicer/VTK/pull/17.

8

So the NULL handling bug is actually already fixed on VTK9.

I have noticed however that running the following code does generate a lot of errors:

shNode = slicer.vtkMRMLSubjectHierarchyNode.GetSubjectHierarchyNode(slicer.mrmlScene)
itemID = shNode.CreateFolderItem(shNode.GetSceneItemID(), 'Test')

with slicer.util.NodeModify(shNode):
  shNode.SetItemName(itemID, 'NewName')
  shNode.SetItemAttribute(itemID, 'NewAttribute', 'a')

Errors:

double __cdecl qSlicerSubjectHierarchyFolderPlugin::canOwnSubjectHierarchyItem(__int64) const : Input item is invalid! 
double __cdecl qSlicerSubjectHierarchyChartsPlugin::canOwnSubjectHierarchyItem(__int64) const : Invalid input item 
double __cdecl qSlicerSubjectHierarchyPlotChartPlugin::canOwnSubjectHierarchyItem(__int64) const : Invalid input item 
GetItemDataNode: Failed to find subject hierarchy item by ID 0


double __cdecl qSlicerSubjectHierarchyVolumesPlugin::canOwnSubjectHierarchyItem(__int64) const : Invalid input item 
double __cdecl qSlicerSubjectHierarchyLabelMapsPlugin::canOwnSubjectHierarchyItem(__int64) const : Invalid input item 
double __cdecl qSlicerSubjectHierarchyDiffusionTensorVolumesPlugin::canOwnSubjectHierarchyItem(__int64) const : Invalid input item 
double __cdecl qSlicerSubjectHierarchyDICOMPlugin::canOwnSubjectHierarchyItem(__int64) const : Invalid input item 
double __cdecl qSlicerSubjectHierarchyMarkupsPlugin::canOwnSubjectHierarchyItem(__int64) const : Invalid input item 
double __cdecl qSlicerSubjectHierarchyModelsPlugin::canOwnSubjectHierarchyItem(__int64) const : Invalid input item 
double __cdecl qSlicerSubjectHierarchySceneViewsPlugin::canOwnSubjectHierarchyItem(__int64) const : Invalid input item 
double __cdecl qSlicerSubjectHierarchySegmentationsPlugin::canOwnSubjectHierarchyItem(__int64) const : Invalid input item 
double __cdecl qSlicerSubjectHierarchySegmentsPlugin::canOwnSubjectHierarchyItem(__int64) const : Invalid input item 
double __cdecl qSlicerSubjectHierarchyTablesPlugin::canOwnSubjectHierarchyItem(__int64) const : Invalid input item 
double __cdecl qSlicerSubjectHierarchyTransformsPlugin::canOwnSubjectHierarchyItem(__int64) const : Invalid input item 
SetItemOwnerPluginName: Failed to find subject hierarchy item by ID 0

@cpinter, @jcfr: I think the SH node should fail silently when you request an invalid item ID. What do you think ?

9

I have been thinking about this as well, but haven’t made this change, because trying to find a non-existent item is an operation that probably leads to errors, so logging an error can be useful.

I think instead of making this fail silently you could investigate why these operations want to access this non-existent item.

10

In this case, it’s because the SubjectHierarchyItemModifiedEvent is called with NULL calldata. This happens whenever you group modifications of the Subject Hierarchy Node together with a StartModify()/EndModify() (see here: https://github.com/Slicer/Slicer/blob/master/Libs/MRML/Core/vtkMRMLNode.h#L478).

As I see it, we have two options:

  • We simply ignore any event passed with NULL calldata. Users will have to be careful when using batch updated with the Subject Hierachy Node and call ItemModified() themselves if needed.
  • We could re-implement vtkMRMLNode::InvokeCustomModifiedEvent and vtkMRMLNode::InvokePendingModifiedEvent in the Subject Hierarchy node to handle passing the item ID

I don’t feel that any of these are ideal, so feel free to suggest other ideas !

11

Normally, any custom modified events with NULL calldata means that multiple updates happened, so a full update is needed. Therefore, for me the obvious solution is to handle this as a full update.

Batch processing is always has overhead, so it should only be used if either A. it is important to not invoke (custom) modified events before multiple modifications are completed; or B. to improve performance by replacing many small updates by one big update. If you find that You may decide to call Start/EndModify only when many items are modified (similarly how it is done here, when batch processing mode is used only when more than a few models had to be updated).