Hello. I’m starting this thread to channel the discussions I’ve had with @lassoan, @pieper, @jcfr and @Sunderlandkyl regarding the creation of new markups (possibly in modules other than Makups).
As many other components in Slicer, Markups are extensible. Currently there are two approaches to create new markups:
Adding the new markups to the Markups module. This approach has no limitations in terms of what type of markups you can add, however, systematically adding all markups here will make the module grow to an unmanageable size for the user (and the developers); only markups that are of general interest for the community should be added here. Furthermore, markups added in the Markups module should be properly developed and well tested, which might mean long integration time.
Adding derived markups in other modules. It is possible to include new markups in modules other than Markups. The limitation of this approach is that the new markups need to be derivative types of the already existing markups in Markup. A new markup derived from vtkMRMLMakupsNode won’t be handled properly, while a new markup derived from vtkMRMLMarkupsCurveNode wil be handled properly. The reason for this is that the handling of the markups in the Markups module is hardcoded to the existing types in the Markups module (and implicitly the derivative types).
An improvement to this could be to modify the Markups infrastructure to allow other modules to register their own markups. One way to to this could be to enable a markups registration functionality in one of the current components (vtkSlicerMarkupsLogic or qSlicerMarkupsModule) and have the components where the markup types are harcoded (vtkMarkupsDisplayableManager, qSlicerWidgets and qMRMLWidgets) adhering to the list of registered markups.
This sounds great @RafaelPalomar. It’s a recurring issue/pattern in Slicer that we need to generalize from hard coded to extensible functionality (that’s why there still a Modules/Core directory even though all modules since the very early days have been created as Loadable for consistency with extension-provided modules). So it would be good to review the various examples of this and see which patterns make the most sense. Off hand I can think of Modules, Segment Editor Effects, DICOMPlugins, Subject Hierarchy Plugins, SampleDataSources at least and they all use slightly different mechanisms.
It would be interesting to consider whether it will be possible to write a Markup extension in Python or if C++ will be required.
In the current Slicer master we haven’t implemented pluggable markups yet. @Sunderlandkyl is now working on adding this, adding a special curve for brain cortical surface segmentation in an extension.
After C++ implementation is complete, we can add scripted markups node base classes (widget, representation, and measurement) that can be used to define custom markups in Python.
@RafaelPalomar I think the best would be if you could work directly with @sunderlandkyl, telling him what your requirements will be, so that he can take those into account when designing the pluggable interface.
@pieper, @lassoan, @Sunderlandkyl I have started looking at the vtkMRMLMarkupsDisplayableManager to replace the hard-coded markups by a more generic approach.
As I see it , we would need to replace the current Focus member from:
std::set<std::string> Focus; // keep registry of nodes
to:
std::map<std::string, std::string> Focus; // keep registry of nodes and associated vtkmrmlwidgets
Then we would need to add a function to register the markups nodes and the associated widgets. This is useful because there are functions (e.g., CreateWdiget) creating widgets of a given type according to the node being processed. An alternative to this could be to keep the associated widget in the vtkMRMLMarkupsNode. Any thoughts?
The second point to discuss here is a bit more tricky. We need to resolve the MRMLWidget sub-type in runtime. I have tried with:
however, CreateInstance returns always nullptr. I suppose we need to enable a factory for the vtkSlicerMarkupsWidget sub-types. It seems the MRMLDisplayableManagers do something similar. Is this the way to go?
Usually we don’t rely on the vtkObjectFactory but pass a factory method when we register a custom class (see for example how custom segment editor effects or IO plugins are registered). This may be more robust and flexible than creating a new instance based purely on class name.
For example, if we allow users to create Python scripted widgets they would all use the same C++ base class and so the vtkObjectFactory probably would not work (or we would need to invent some new design).
The basic idea is to add a factory method to the vtkSlicerMarkupsWidget and derivatives to specify how the instantiation of a given widget should be. The DM was extended with a registration function and the hard-coding of markups was replaced by code that checks the registered nodes.
You will notice that in the constructor there are still hard-coded values. The DM does not need this per se, but as I see it, the registration of the DM on the module setup is based on classes and not instances:
// Register displayable managers (same displayable manager handles both slice and 3D views)
vtkMRMLSliceViewDisplayableManagerFactory::GetInstance()->RegisterDisplayableManager("vtkMRMLMarkupsDisplayableManager");
vtkMRMLThreeDViewDisplayableManagerFactory::GetInstance()->RegisterDisplayableManager("vtkMRMLMarkupsDisplayableManager");
This means that the configuration of the DM needs to happen in the constructor. Is that how we want it to be?
I’m going to try implementing a new markup in an external module and see how it plays with these changes.
It looks to me that qSlicerMarkupsWidget, qSlicerSubjectHierarchyMarkupsPlugin, vtkMRMLMarkupsFiducialStorageNode and vtkSlicerMarkupsLogic have hard-coded values and would also require changes.
@lassoan, @pieper I generated a new markup in an external module (https://github.com/ALive-research/Slicer-LiverAnalysis) to test the changes I made in #5349. So far, creating a simple inherited MRML node, MRMLDM and SlicerLogic makes possible adding entirely new markups to the scene.
While this helps the extensibility of markups, I don’t think this is still a completely pluggable architecture. For that it should not be needed to define a new MRMLDM. As mentioned earlier in this thread, this seems to me a limitation of how the MRMLDMs get registered (class-based and not object based).
I’ll continue working on applying similar changes to the widgets side of the Markups, so new markups will be handled by the GUI.
Let me know if you think this is going in the right direction.
I have tested it having an external module defining its own markup and seems to work fine.
I’m not that knowledgeable on the Python infrastructure, but I would think if one can define a vtkMRMLMarkups node and a vtkSlicerMarkupsWidget (which is a vtkMRMLAbstractWidget) in Python, the proposed changes will enable pluggable markups in Python too.
Let me know if you have time tomorrow in the devs meeting to have a discussion on this.
Looks very nice. Yes, we should have time to discuss this in tomorrow’s meeting.
I’m wondering if we want to support the case where you could more than one possible widget type to operate on a markup node. For example we might want to have different widgets for different manipulation modes. For this maybe we’d want to use node attributes like we do for node comboboxes.
To follow up, associated pull request Slicer PR-5349 has been integrated.
Many thanks to @RafaelPalomar for his patience and persistence during the review process, and also thanks to everyone who participated to the weekly hangouts and helped review and discuss how to best integrate this feature.
I’ve created a new extension based on the Bézier surface markup by @RafaelPalomar. I added the NURBS interpolation type to be able to create surfaces more intuitively and made some improvements and generalizations.
We are planning to further improve this markup (fitting to model/segmentation, shared edges, etc.), and expect it to be around for a while.
It is all ready (see PR), however, questions have arisen about its name. Currently it is called GridSurface, but it does not describe its purpose too well, and seems to be too technical. I’d like to ask for your help in finding an appropriate name.
Some of the ideas so far:
Surface patch
Surface markup
Surface warp
Curved surface
Curved plane
Here’s a very short video that shows how it looks:
If the term “grid” is not in the name, would that make it indistinguishable from some other future surface that starts out as a circle instead of a rectangular grid? Or would whether it starts out as rectangle or circle be a property of the same markup type?
I would suggest avoiding the use of “markup” in the name as in “surface markup”. This is because we refer to things as “markups line” or “markups curve” as that it is controlled by the Markups module. So it would be weird to then say this is a “markups surface markup”. Instead I would call it simply a “surface” or “curved surface” to more clearly imply that it is more likely not going to be flat in its final form.
I think one of the things we’ve learned about the markups module is that it is going to need to be a general module. So markups have general names like “Line” which don’t describe their purpose. A “Line” could be used as a Ruler or as a axis of rotation. There are many applications.
In this case, the surface could be used as a patch for something, but it could also be used for something else. Whichever name is chosen, specific application should not be implied in the name.
The name of the extension and the name of the markup are two different things! I’d of course not include the word “markup” in the markup name, only in the extension name so that the people browsing the Extension Manager have an idea where it fits.
What I have currently is an early version of what is finally imagined. Yes, we do plan to add an approximation feature that can be used to fit the surface to an existing model/segmentation.
for the Slicer-Liver module we have markups that are “ephimeral”. These markups are temporarily created and can lead to generation of other markpus (in Slicer-Liver we use them to initialize the Bezier Surface that will specify a resection trajectory).
Right now, registration of node, registration of markups widget and registration for adding the UI button happens all in vtkSlicerMarkupsLogic::RegisterMarkupsNode. The problem for us is that we don’t want to show these ephimeral markups in the UI; they don’t make sense by themselves alone. For more flexibility, I would suggest to split the registration of the markups in different functions. So you can support the case of a fully working markup that does not show up in the Markups UI module (add as button). We could still keep the RegisterMarkupsNode function that does it all so the API does not change. What do you think?
