# Should we use more VTK object "factory" methods?

**URL:** <https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092>\
**Category:** Development\
**Created:** [April 24, 2023, 2:56pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092 "2023-04-24T14:56:24Z")\
**Posts on this page:** 16\
**Page:** 1

<div class="post-metadata">

**Author:** ![lassoan](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/lassoan/32/13_2.png) [@lassoan](https://discourse.slicer.org/u/lassoan)\
**Post date:** [April 24, 2023, 2:56pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092/1 "2023-04-24T14:56:24Z")

</div>

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:

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

```

We can use this:

```auto
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](https://github.com/Slicer/Slicer/pull/6958). 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

---

<div class="post-metadata">

**Author:** ![pieper](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/pieper/32/8_2.png) [@pieper](https://discourse.slicer.org/u/pieper)\
**Post date:** [April 24, 2023, 5:54pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092/2 "2023-04-24T17:54:15Z")

</div>

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.

---

<div class="post-metadata">

**Author:** ![jcfr](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/jcfr/32/17825_2.png) [@jcfr](https://discourse.slicer.org/u/jcfr)\
**Post date:** [April 24, 2023, 6:32pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092/3 "2023-04-24T18:32:14Z")

</div>

> 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:

- [https://slicer.readthedocs.io/en/latest/developer\_guide/advanced\_topics.html#factory-methods](https://slicer.readthedocs.io/en/latest/developer_guide/advanced_topics.html#factory-methods)
- [https://github.com/Kitware/VTK/blob/master/Documentation/Doxygen/PythonWrappers.md#wrapper-hints-wrapper-hints](https://github.com/Kitware/VTK/blob/master/Documentation/Doxygen/PythonWrappers.md#wrapper-hints-wrapper-hints)

---

<div class="post-metadata">

**Author:** ![cpinter](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/cpinter/32/7995_2.png) [@cpinter](https://discourse.slicer.org/u/cpinter)\
**Post date:** [April 24, 2023, 7:32pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092/4 "2023-04-24T19:32:23Z")

</div>

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!

---

<div class="post-metadata">

**Author:** ![pieper](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/pieper/32/8_2.png) [@pieper](https://discourse.slicer.org/u/pieper)\
**Post date:** [April 24, 2023, 7:36pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092/5 "2023-04-24T19:36:20Z")

</div>

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.

---

<div class="post-metadata">

**Author:** ![lassoan](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/lassoan/32/13_2.png) [@lassoan](https://discourse.slicer.org/u/lassoan)\
**Post date:** [April 24, 2023, 8:20pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092/6 "2023-04-24T20:20:10Z")

</div>

> [@jcfr](#):
>
> > We may consider adding factory method variants for existing methods
> 
> Which name were you thinking ?

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:

```auto
int vtkMRMLTransformNode::GetMatrixTransformToParent(vtkMatrix4x4* matrix)

```

we could use:

```auto
VTK_NEWINSTANCE vtkMatrix4x4* vtkMRMLTransformNode::CreateMatrixTransformToParentCopy()

```

Instead of:

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

```

we could use:

```auto
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:

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

```

What would be the factory method name?

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

```

> [@pieper](#):
>
> Are we going to be changing the behavior of the existing API with this?

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:

```auto
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.

---

<div class="post-metadata">

**Author:** ![pieper](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/pieper/32/8_2.png) [@pieper](https://discourse.slicer.org/u/pieper)\
**Post date:** [April 24, 2023, 8:38pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092/7 "2023-04-24T20:38:11Z")

</div>

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.

---

<div class="post-metadata">

**Author:** ![lassoan](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/lassoan/32/13_2.png) [@lassoan](https://discourse.slicer.org/u/lassoan)\
**Post date:** [April 24, 2023, 9:06pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092/8 "2023-04-24T21:06:11Z")

</div>

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

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

```

→

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

```

Quite misleading.

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

```

It is much better.

### Example 2

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

```

→

```auto
matrix = transformNode.GetNewMatrixTransformToParent()

```

Not very clear. Maybe OK.

### Example 3

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

```

→

```auto
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.

---

<div class="post-metadata">

**Author:** ![jcfr](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/jcfr/32/17825_2.png) [@jcfr](https://discourse.slicer.org/u/jcfr)\
**Post date:** [April 24, 2023, 9:12pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092/9 "2023-04-24T21:12:57Z")

</div>

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

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

```

And to understand the usage context:

```plaintext
$ 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

```

---

<div class="post-metadata">

**Author:** ![jcfr](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/jcfr/32/17825_2.png) [@jcfr](https://discourse.slicer.org/u/jcfr)\
**Post date:** [April 24, 2023, 9:20pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092/10 "2023-04-24T21:20:16Z")

</div>

And here are the current Slicer core APIs already using `VTK_NEWINSTANCE`

```plaintext
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(); }

```

---

<div class="post-metadata">

**Author:** ![jcfr](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/jcfr/32/17825_2.png) [@jcfr](https://discourse.slicer.org/u/jcfr)\
**Post date:** [April 24, 2023, 9:28pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092/11 "2023-04-24T21:28:57Z")

</div>

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

```diff
-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

```

---

<div class="post-metadata">

**Author:** ![jamesobutler](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/jamesobutler/32/7511_2.png) [@jamesobutler](https://discourse.slicer.org/u/jamesobutler)\
**Post date:** [April 24, 2023, 9:32pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092/12 "2023-04-24T21:32:56Z")

</div>

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.

---

<div class="post-metadata">

**Author:** ![jcfr](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/jcfr/32/17825_2.png) [@jcfr](https://discourse.slicer.org/u/jcfr)\
**Post date:** [April 24, 2023, 9:36pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092/13 "2023-04-24T21:36:07Z")

</div>

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

---

<div class="post-metadata">

**Author:** ![lassoan](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/lassoan/32/13_2.png) [@lassoan](https://discourse.slicer.org/u/lassoan)\
**Post date:** [April 24, 2023, 10:00pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092/14 "2023-04-24T22:00:21Z")

</div>

> [@jamesobutler](#):
>
> I find it a little confusing at first that “GetNew” equals “Create”.

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.

---

<div class="post-metadata">

**Author:** ![jcfr](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/jcfr/32/17825_2.png) [@jcfr](https://discourse.slicer.org/u/jcfr)\
**Post date:** [April 24, 2023, 10:45pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092/15 "2023-04-24T22:45:09Z")

</div>

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

```diff
-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`)

```diff
-vtkMRMLScene::GetNodesByClass
+vtkMRMLScene::GetNodeCollectionByClass

```

---

<div class="post-metadata">

**Author:** ![lassoan](https://sea2.discourse-cdn.com/flex002/user_avatar/discourse.slicer.org/lassoan/32/13_2.png) [@lassoan](https://discourse.slicer.org/u/lassoan)\
**Post date:** [April 25, 2023, 6:21pm UTC](https://discourse.slicer.org/t/should-we-use-more-vtk-object-factory-methods/29092/16 "2023-04-25T18:21:06Z")

</div>

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.