Add a parameter set selector and append the result in a table.

[chir-set]

StenosisMeasurement3D

• create an MRML parameter node class
• do not create a default parameter node in the selector
• enforce selection of a parameter set
• use the parameter node in logic
• append the results in an MRML table
• improve existing code.

[chir-set]

Hello @lassoan ,

Please consider this PR with the main purpose of adding a parameter node to the StenosisMeasurement3D module. The parameter node is mainly based on vtkMRMLCropVolumeParametersNode.

Waiting for your comments.

Regards.

[lassoan]

Looks good overall, I don't have any problems if you merge it as is. But I added a couple of comments inline that would improve the design a bit.

[lassoan]

I don't think vtkSetObjectMacro/vtkGetObjectMacro is compatible with vtkWeakPointer, but even if it does not crash or behaves incorrectly, it just does not make sense.

If you use vtkSetObjectMacro/vtkGetObjectMacro then it means that vtkMRMLStenosisMeasurement3DParameterNode owns the object (the parameter node keeps a reference to the object, so the object will not be deleted until the parameter node releases it) and the vtkMRMLStenosisMeasurement3DParameterNode should use a simple pointer (that is initialized to nullptr here).

If you use a vtkSmartPointer then you can use vtkSetMacro/vtkGetMacro and the parameter node owns the object.
If you use a vtkWeakPointer then you can use vtkSetMacro/vtkGetMacro and some other object must keep a reference to the object.

[chir-set]

Changed for vtkSmartPointer.

But vtkGet/SetMacro does not compile.

[lassoan]

What is the error message? If adding #include <vtkSmartPointer.h> does not solve the issue then you can investigate further (copy the error message here, maybe I have an idea). But often we don't need get/set functions, as you can set the variable with a single operator=.

[chir-set]

Here is a typical error message:

In file included from /home/user/tmp/Slicer/SlicerExtension-VMTK/StenosisMeasurement3D/MRML/vtkMRMLStenosisMeasurement3DParameterNode.cxx:1:
/home/user/tmp/Slicer/SlicerExtension-VMTK/StenosisMeasurement3D/MRML/vtkMRMLStenosisMeasurement3DParameterNode.h:62:5: error: no viable overloaded '='
62 | vtkSetMacro(OutputWallOpenPolyData, vtkPolyData);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/arc/src/Slicer-SuperBuild9/VTK/Common/Core/vtkSetGet.h:134:18: note: expanded from macro 'vtkSetMacro'
134 | this->name = _arg;
| ~~~~~~~~~~ ^ ~~~~
/home/arc/src/Slicer-SuperBuild9/VTK/Common/Core/vtkSmartPointer.h:144:20: note: candidate function not viable: no known conversion from 'vtkPolyData' to 'const vtkSmartPointer' for 1st argument
144 | vtkSmartPointer& operator=(const vtkSmartPointer& r)
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~
/home/arc/src/Slicer-SuperBuild9/VTK/Common/Core/vtkSmartPointer.h:151:20: note: candidate template ignored: could not match 'vtkSmartPointer' against 'vtkPolyData'
151 | vtkSmartPointer& operator=(const vtkSmartPointer& r)
| ^
/home/arc/src/Slicer-SuperBuild9/VTK/Common/Core/vtkSmartPointer.h:165:20: note: candidate template ignored: could not match 'vtkNew' against 'vtkPolyData'
165 | vtkSmartPointer& operator=(const vtkNew& r)
| ^
/home/arc/src/Slicer-SuperBuild9/VTK/Common/Core/vtkSmartPointer.h:178:20: note: candidate template ignored: could not match 'U ' against 'vtkPolyData'
178 | vtkSmartPointer& operator=(U r)
| ^
In file included from /home/user/tmp/Slicer/SlicerExtension-VMTK/StenosisMeasurement3D/MRML/vtkMRMLStenosisMeasurement3DParameterNode.cxx:1:
/home/user/tmp/Slicer/SlicerExtension-VMTK/StenosisMeasurement3D/MRML/vtkMRMLStenosisMeasurement3DParameterNode.h:63:5: error: no viable conversion from returned value of type 'vtkSmartPointer' to function return type 'vtkPolyData'
63 | vtkGetMacro(OutputWallOpenPolyData, vtkPolyData)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/arc/src/Slicer-SuperBuild9/VTK/Common/Core/vtkSetGet.h:156:12: note: expanded from macro 'vtkGetMacro'
156 | return this->name;
| ^~~~~~~~~~
/home/arc/src/Slicer-SuperBuild9/VTK/Common/DataModel/vtkPolyData.h:757:3: note: candidate constructor not viable: no known conversion from 'vtkSmartPointer' to 'const vtkPolyData &' for 1st argument
757 | vtkPolyData(const vtkPolyData&) = delete;
| ^ ~~~~~~~~~~~~~~~~~~
/home/arc/src/Slicer-SuperBuild9/VTK/Common/Core/vtkSmartPointer.h:197:3: note: candidate function
197 | operator T*() const noexcept { return static_cast<T*>(this->Object); }
| ^
5 errors generated.

I'll probably not use the macro.

[lassoan]

You may need to add #include <vtkPolyData.h> as well.

[chir-set]

vtkPolyData.h is already included. vtkSmartPointer.h and vtkSetGet.h do not help.

I'm using clang version 18.1.8, don't know if it's the problem.

Dropping the macro is probably the simplest solution.

[chir-set]

Grepping through the Slicer code base, it seems that vtkSet/GetMacro is always used for data types like int, double... like here, and vtkSet/GetObjectMacro is always used for objects created from high-level classes like here.

[lassoan]

We always need to consider the GUI as optional. The module should be fully functional without instantiating a GUI widget. Therefore, you need to put in the logic things like observing some node and updating the scene content when the observed node is modified.

There are convenience features in module logic classes that make it relatively simple to observe modifications of all nodes of a certain type (see vtkSetAndObserveMRMLNodeEventsMacro and ProcessMRMLNodesEvents and how it is used for example in vtkSlicerMarkupsLogic).

[chir-set]

Using vtkObserveMRMLNodeEventsMacro in ProcessMRMLNodesEvents

How does it differ from vtkSetAndObserveMRMLNodeEventsMacro?

[lassoan]

vtkSetAndObserveMRMLNodeEventsMacro is useful when you want to keep a pointer to a node and observe events that the node invokes. The macro does everything you need: removes observations from the previously observed node, releases reference from the old node, adds observations and reference to the newly selected node.

vtkObserveMRMLNodeEventsMacro just adds observations to a node.

[chir-set]

Thank you for this insightful review, I'll fix all these.

[chir-set]

Hi @lassoan ,

I made all the changes, moving much code in logic. Testing from Python is successful. I'm ready to change other things per your instructions.

Thanks and regards.

[lassoan]

Great! I'll review your changes in details, so if you have any specific questions then let me know.