Should we use more VTK object "factory" methods?

Development
1

We call those methods that create and return a new VTK object “factory method”. Previously, we avoided using such methods because it was too easy to forget to take ownership of the new object (and delete it when no longer used).

However, now the chance of factory method misuse is much lower. 1-2 years ago VTK introduced the VTK_NEWINSTANCE hint that makes ownership transfer automatic when the method is used from Python. Since most beginner Slicer programmers now use Python now, we could expect much less memory leaks.

Using factory methods allow much simpler usage. For example, instead of this:

mesh = vtk.vtkPolyData()
getNode('R').CreateROIBoxPolyDataWorld(mesh)
slicer.modules.models.logic().AddModel(mesh)

We can use this:

slicer.modules.models.logic().AddModel( getNode('R').CreateROIBoxPolyDataWorld() )

It is not just shorter but also much easier to write because the user does not have to know what type of output object need to be instantiated.

We could use factory methods again for most new methods that create a single new VTK object as output. For example, there is a pull request with a new method like this here. We may consider adding factory method variants for existing methods, too.

What do you think? Does the much simpler usage in Python justifies the small risk of misusing of factory methods in C++?

@pieper @jcfr @cpinter @jamesobutler @Sunderlandkyl

2

I like the idea of a shorter cleaner interface on the python side. It would be nice for all methods with similar names to behave the same with respect to reference counts.

3

use factory methods again for most new methods that create a single new VTK object as output.

Agreed.

We may consider adding factory method variants for existing methods

Which name were you thinking ?

Instead of introducing new method, I am wondering if we could:

  • toggle the new behavior on a case by case
  • report a warning if old behavior is being used

Before adding such complexity, we should also assess how prevalent the use of factory methods (and associated hack consisting in calling UnRegister is).

References:

4

Great to know! This will make life easier for sure. This decorator should be added to similar functions, starting with the ones in the scene returning vtkCollection. Thanks for bringing this up!

5

Are we going to be changing the behavior of the existing API with this? Seems like it could lead to leaks or double frees or who knows what.

6

Adding new method variants should not be difficult.

Current non-factory methods usually start with Get... and take the output object pointer as an argument.

In VTK, a public method is factory method if an only if its name starts with Create.... (There are just a handful of exceptions: New... is used for iterators and Make... for keys, transform, camera, and render window interactor). So, name of any new factory method in VTK classes in Slicer should start with Create....

Since the old methods names start with Get... and have a different argument list, the method signature will be different, so there would be no backward compatibility issues.

The new method names could be Create...Copy when we want to get an object but it is only feasible to get a copy (because we don’t want to expose an internal object or the object is not stored internally). For example:

Instead of:

int vtkMRMLTransformNode::GetMatrixTransformToParent(vtkMatrix4x4* matrix)

we could use:

VTK_NEWINSTANCE vtkMatrix4x4* vtkMRMLTransformNode::CreateMatrixTransformToParentCopy()

Instead of:

bool vtkMRMLSegmentationNode::GetBinaryLabelmapRepresentation(const std::string segmentId, vtkOrientedImageData* outputBinaryLabelmap);

we could use:

VTK_NEWINSTANCE vtkOrientedImageData* vtkMRMLSegmentationNode::CreateBinaryLabelmapRepresentationCopy(const std::string segmentId);

A complication is that in Slicer some factory methods don’t start with Create.... For these we could invent a new name and deprecate the old one. However, this can be quite difficult.

For example:

Current non-conform factory method name:

nodes = slicer.mrmlScene.GetNodesByClass('vtkMRMLScalarVolumeNode')

What would be the factory method name?

nodes = slicer.mrmlScene.CreateNodeCollectionByClass('vtkMRMLScalarVolumeNode')
nodes = slicer.mrmlScene.CreateNodeListByClass('vtkMRMLScalarVolumeNode')
nodes = slicer.mrmlScene.CreateCollectionOfNodesByClass('vtkMRMLScalarVolumeNode')
...?

Yes, ideally we should add this to all current factory methods. The issue is that currently when a factory method is called from Python then Unregister is called, which would crash Slicer after if we simply add the VTK_NEWINSTANCE hint.

Maybe we could do it in phases:

  1. Add CreateAndTake... method variant (only expose in Python)

For example:

node = slicer.mrmlScene.CreateAndTakeNodeByClass('vtkMRMLMultiVolumeNode')
  1. Remove Python wrapping from the original Create... method

In this phase Python code that is not updated to CreateAndTake... would fail. This should force every maintained extensions to switch to CreateAndTake...

  1. Re-enable Python wrapping of Create..., but now with VTK_NEWINSTANCE hint enabled.

  2. Remove CreateAndTake... methods

If we don’t want to rush developers then we would need to spend at least 6 months in each phase.

7

I’m not a fan of changing the behavior of existing methods in such a subtle and possibly dangerous way. Especially not multiple times over many months. That would be very hard to maintain since you would have version-specific scripts or scripts that would behave differently on different versions.

I’d much prefer introducing new methods with a clear naming convention to expose the new functionality. If we think the old way is really dangerous we could deprecate the methods but if they work I would leave them be and not even warn when they are used.

To me, CreateAndTake is a bit clunky but it seems consistent with the meaning and the usage of take in other APIs. GetNew might make sense, since we are talking about new instances. This would fit nicely beside the existing Get classes.

8

As a maintainer of many extension I hate API changes, too. It is just a time sink, with marginal benefit for existing code. At the same time, for new users it would be nice to avoid having magical Unregister(0) calls. Maybe we could just do step 1 and maybe 2 (and put off step 3 and 4 indefinitely).

For this, we would need alternative method name that could remain in the code long term.

CreateAndTake... is meant to be clunky - optimized for ease of search-and-replace, and ugly enough that people are happy to get rid of it when they can.

GetNew... sounds quite good. It is very nice in that when the user searches for Get... method names with autocomplete then it shows up there. However, it would complicate the factory method definition: GetNew... would be yet another pattern that C++ users would need to learn to recognize. It may lead to confusing names if used as a drop-in replacement for Get...:

Example 1

nodes = slicer.mrmlScene.GetNodesByClass('vtkMRMLScalarVolumeNode')

→ 

nodes = slicer.mrmlScene.GetNewNodesByClass('vtkMRMLScalarVolumeNode')

Quite misleading.

nodes = slicer.mrmlScene.GetNewCollectionOfNodesByClass('vtkMRMLScalarVolumeNode')

It is much better.

Example 2

matrix = vtk.vtkMatrix4x4()
transformNode.GetMatrixTransformToParent(matrix)

→ 

matrix = transformNode.GetNewMatrixTransformToParent()

Not very clear. Maybe OK.

Example 3

outputBinaryLabelmap = vtk.vtkOrientedImageData()
segmentationNode.GetBinaryLabelmapRepresentation(segmentId, outputBinaryLabelmap);

→ 

outputBinaryLabelmap = segmentationNode.GetNewBinaryLabelmapRepresentation(segmentId);

This is probably OK as is.

Overall GetNew... is not too bad, and in most cases probably better than Create...Copy (because it preserves the Get... prefix), but maybe someone else has even better name suggestion.

9

Listed below are the methods that require to use UnRegister when used in Slicer python scripts:

vtkMRMLScene::GetNodesByClass
vtkMRMLScene::CreateNodeByClass
vtkSlicerSegmentationsModuleLogic::CreateOrientedImageDataFromVolumeNode
vtkSlicerSegmentationsModuleLogic::CreateSegmentFromModelNode
vtkSlicerSegmentationsModuleLogic::CreateSegmentFromLabelmapVolumeNode
vtkSlicerSegmentationsModuleLogic::CreateOrientedImageDataFromVolumeNode
vtkSlicerCLIModuleLogic::CreateNode

And to understand the usage context:

$ ack --py UnRegister -B1
Libs/MRML/Core/Documentation/generate_default_color_node_property_table.py
24-nodes = slicer.mrmlScene.GetNodesByClass("vtkMRMLColorNode")
25:nodes.UnRegister(slicer.mrmlScene)

Base/Python/slicer/util.py
1508-    nodes = slicer.mrmlScene.GetNodesByClass(className)
1509:    nodes.UnRegister(slicer.mrmlScene)

Base/Python/slicer/ScriptedLoadableModule.py
291-        node = slicer.mrmlScene.CreateNodeByClass("vtkMRMLScriptedModuleNode")
292:        node.UnRegister(None)  # object is owned by the Python variable now

Modules/Loadable/Segmentations/Testing/Python/SegmentationWidgetsTest1.py
184-        mrOrientedImageData = vtkSlicerSegmentationsModuleLogic.vtkSlicerSegmentationsModuleLogic.CreateOrientedImageDataFromVolumeNode(mrVolumeNode)
185:        mrOrientedImageData.UnRegister(None)

Modules/Loadable/Segmentations/Testing/Python/SegmentationsModuleTest1.py
342-        modelSegment = slicer.vtkSlicerSegmentationsModuleLogic.CreateSegmentFromModelNode(bodyModelNode)
343:        modelSegment.UnRegister(None)  # Need to release ownership
--
371-        labelSegment = slicer.vtkSlicerSegmentationsModuleLogic.CreateSegmentFromLabelmapVolumeNode(allSegmentsLabelmapNode)
372:        labelSegment.UnRegister(None)  # Need to release ownership
--
417-        modelSegmentTranformed = slicer.vtkSlicerSegmentationsModuleLogic.CreateSegmentFromModelNode(bodyModelNodeTransformed, modelTransformedImportSegmentationNode)
418:        modelSegmentTranformed.UnRegister(None)  # Need to release ownership

Modules/Loadable/Segmentations/EditorEffects/Python/SegmentEditorMaskVolumeEffect.py
356-            img = slicer.modules.segmentations.logic().CreateOrientedImageDataFromVolumeNode(maskVolumeNode)
357:            img.UnRegister(None)

Modules/Loadable/SubjectHierarchy/Testing/Python/SubjectHierarchyCorePluginsSelfTest.py
218-        segmentationNodes = slicer.mrmlScene.GetNodesByClass('vtkMRMLSegmentationNode')
219:        segmentationNodes.UnRegister(None)

Modules/Scripted/DICOMLib/DICOMUtils.py
506-            nodeCollection = slicer.mrmlScene.GetNodesByClass(nodeType)
507:            nodeCollection.UnRegister(None)
--
515-            nodeCollection = slicer.mrmlScene.GetNodesByClass(nodeType)
516:            nodeCollection.UnRegister(None)

Modules/Scripted/ImportItkSnapLabel/ImportItkSnapLabel.py
63-            colorNode = slicer.mrmlScene.CreateNodeByClass("vtkMRMLColorTableNode")
64:            colorNode.UnRegister(None)  # to prevent memory leaks

Modules/Scripted/DICOMPlugins/DICOMVolumeSequencePlugin.py
46-        browserNodes = slicer.mrmlScene.GetNodesByClass('vtkMRMLSequenceBrowserNode')
47:        browserNodes.UnRegister(None)
--
64-        # (a reference is still kept by createDicomSeriesParameterNode Python variable).
65:        createDicomSeriesParameterNode.UnRegister(None)

Modules/Scripted/SegmentEditor/SegmentEditor.py
82-            segmentEditorNode = slicer.mrmlScene.CreateNodeByClass("vtkMRMLSegmentEditorNode")
83:            segmentEditorNode.UnRegister(None)

Applications/SlicerApp/Testing/Python/ScenePerformance.py
258-            newNode = node.CreateNodeInstance()
259:            newNode.UnRegister(node)

Applications/SlicerApp/Testing/Python/MRMLCreateNodeByClassWithSetReferenceCountMinusOne.py
5-    n = slicer.mrmlScene.CreateNodeByClass('vtkMRMLViewNode')
6:    n.UnRegister(None)  # the node object is now owned by n Python variable therefore we can release the reference that CreateNodeByClass added
10

And here are the current Slicer core APIs already using VTK_NEWINSTANCE

ack VTK_NEWINSTANCE -A1
Libs/MRML/Core/vtkMRMLMeasurement.h
88:  VTK_NEWINSTANCE
89-  virtual vtkMRMLMeasurement* CreateInstance() const = 0;

Libs/MRML/Core/vtkMRMLStaticMeasurement.h
38:  VTK_NEWINSTANCE
39-  vtkMRMLMeasurement* CreateInstance() const override { return vtkMRMLStaticMeasurement::New(); }

Modules/Loadable/Markups/MRML/vtkMRMLMarkupsJsonStorageNode.h
86:  VTK_NEWINSTANCE
87-  vtkMRMLMarkupsJsonElement* ReadMarkupsFile(const char* filePath);

Modules/Loadable/Markups/MRML/vtkMRMLMeasurementArea.h
37:  VTK_NEWINSTANCE
38-  vtkMRMLMeasurement* CreateInstance() const override { return vtkMRMLMeasurementArea::New(); }

Modules/Loadable/Markups/MRML/vtkMRMLMeasurementAngle.h
37:  VTK_NEWINSTANCE
38-  vtkMRMLMeasurement* CreateInstance() const override { return vtkMRMLMeasurementAngle::New(); }

Modules/Loadable/Markups/MRML/vtkMRMLMeasurementLength.h
37:  VTK_NEWINSTANCE
38-  vtkMRMLMeasurement* CreateInstance() const override { return vtkMRMLMeasurementLength::New(); }

Modules/Loadable/Markups/MRML/vtkMRMLMarkupsJsonElement.h
99:  VTK_NEWINSTANCE
100-  vtkCodedEntry* GetCodedEntryProperty(const char* propertyName);
--
105:  VTK_NEWINSTANCE
106-  vtkDoubleArray* GetDoubleArrayProperty(const char* propertyName);
--
115:  VTK_NEWINSTANCE
116-  vtkMRMLMarkupsJsonElement* GetArrayProperty(const char* arrayName);
--
121:  VTK_NEWINSTANCE
122-  vtkMRMLMarkupsJsonElement* GetObjectProperty(const char* objectName);
--
132:  VTK_NEWINSTANCE
133-  vtkMRMLMarkupsJsonElement* GetArrayItem(int childItemIndex);
--
169:  VTK_NEWINSTANCE
170-  vtkMRMLMarkupsJsonElement* ReadFromFile(const char* filePath);

Modules/Loadable/Markups/MRML/vtkMRMLMeasurementVolume.h
37:  VTK_NEWINSTANCE
38-  vtkMRMLMeasurement* CreateInstance() const override { return vtkMRMLMeasurementVolume::New(); }
11

Based on @lassoan proposal above, these new methods with the VTK_NEWINSTANCE attribute could be added:

-vtkMRMLScene::GetNodesByClass
+vtkMRMLScene::GetNewNodeCollectionByClass # Or GetNewCollectionOfNodesByClass
-vtkMRMLScene::CreateNodeByClass
+vtkMRMLScene::GetNewNodeByClass
-vtkSlicerSegmentationsModuleLogic::CreateOrientedImageDataFromVolumeNode
+vtkSlicerSegmentationsModuleLogic::GetNewOrientedImageDataFromVolumeNode
-vtkSlicerSegmentationsModuleLogic::CreateSegmentFromModelNode
+vtkSlicerSegmentationsModuleLogic::GetNewSegmentFromModelNode
-vtkSlicerSegmentationsModuleLogic::CreateSegmentFromLabelmapVolumeNode
+vtkSlicerSegmentationsModuleLogic::GetNewSegmentFromLabelmapVolumeNode
-vtkSlicerSegmentationsModuleLogic::CreateOrientedImageDataFromVolumeNode
+vtkSlicerSegmentationsModuleLogic::GetNewOrientedImageDataFromVolumeNode
-vtkSlicerCLIModuleLogic::CreateNode
+vtkSlicerCLIModuleLogic::GetNewNode
12

I find it a little confusing at first that “GetNew” equals “Create”. The “GetNew” still sounds like you are retrieving an existing object that was recently created. It is something I could learn, but does not immediately make sense upon first reading it.

1 Like
13

CreateNew instead of Create may be more natural then and the word “New” would align with the use of VTK_NEWINSTANCE

14

I agree.

GetNew... name is intuitive when I want to get a property of an object but for some reason it is only available as a copy in a new object (e.g., because it is not safe to expose the internal object or the object is constructed on-the-fly).

Create... name is intuitive when you actually want to create a new object.

Maybe in addition to vtkMRMLScene::CreateNodeByClass we could add a vtkMRMLScene::CreateNodeByClassOwned method? The ...Owned method would only be available in Python and it would use the VTK_NEWINSTANCE hint. The new Python method would start with the same words as the old method, so auto-complete would bring up both (in contrast CreateNew... would not show up together in automcompete along with with CreateNode...). Maybe there could be a better word instead of ...Owned (...Auto, ...Ref, …?).

vtkMRMLMarkupsJsonElement* GetArrayProperty(arrayName) and similar methods in Markups module are very new, we could still change the names without worrying about backward compatibility (only affects a few extensions that use pluggable markups). We could use GetNewArrayFromProperty(arrayName) here.

15

Instead of Owned/Auto/Ref, what about the Instance suffix, this would be needed only for the < 10 methods:

-vtkMRMLScene::CreateNodeByClass
+vtkMRMLScene::CreateNodeInstanceByClass
-vtkSlicerSegmentationsModuleLogic::CreateOrientedImageDataFromVolumeNode
+vtkSlicerSegmentationsModuleLogic::CreateOrientedImageDataInstanceFromVolumeNode
-vtkSlicerSegmentationsModuleLogic::CreateSegmentFromModelNode
+vtkSlicerSegmentationsModuleLogic::CreateSegmentInstanceFromModelNode
-vtkSlicerSegmentationsModuleLogic::CreateSegmentFromLabelmapVolumeNode
+vtkSlicerSegmentationsModuleLogic::CreateSegmentInstanceFromLabelmapVolumeNode
-vtkSlicerSegmentationsModuleLogic::CreateOrientedImageDataFromVolumeNode
+vtkSlicerSegmentationsModuleLogic::CreateOrientedImageDataInstanceFromVolumeNode
-vtkSlicerCLIModuleLogic::CreateNode
+vtkSlicerCLIModuleLogic::CreateNodeInstance

and for the widely used GetNodesByClass (Nodes → NodeCollection)

-vtkMRMLScene::GetNodesByClass
+vtkMRMLScene::GetNodeCollectionByClass
1 Like
16

We discussed this further at the weekly meeting and concluded that we will not change the current API in an incompatible way but add new variants with proper Python decoration.

For existing Create... method names (such as CreateOrientedImageDataFromVolumeNode) we will add a new Python variant with the ...Safe suffix to indicate that it is memory-safe (no leak or invalid reference).

I’ll follow up with pull requests.

1 Like