Proposal: Plugin interface overhaul

[roelj]

Currently we have three types of plugins: brushes, line tools and selection tools. The differences between these types is how to invoke them.

I'd like to simplify the plugin system by making the type irrelevant to the caller. For this, I'd like to propose a single interface each plugin should implement:

void plugin_get_metadata (PluginMetaData **metadata);

void plugin_apply (PluginMetaData *metadata, PixelData *original,
           PixelData *mask, PixelData *selection,
           Coordinate point);

void* plugin_get_property (PluginMetaData *metadata, const char *property);

bool plugin_set_property (PluginMetaData *metadata, const char *property,
              void *value);

bool plugin_set_voxel_at_point (PixelData *mask, PixelData *selection,
                Coordinate point, unsigned int value,
                PixelAction action);

bool plugin_get_voxel_at_point (PixelData *mask, Coordinate point,
                void *value);

void plugin_destroy (PluginMetaData *metadata);

Where PluginMetaData is defined as:

typedef struct
{
  char *name;        /*< The name of the plugin. */
  int version;       /*< The version of the plugin. */
  void *properties;  /*< A pointer to the plugin's properties. */

} PluginMetaData;

When a plugin is loaded, the functions can be called as:

plugin->get_metadata(...);
plugin->apply(...);
plugin->get_property(...);
plugin->set_property(...);
plugin->destroy(...);

Both get_voxel and set_voxel don't have to be called directly.

Essentially, the differences between the types can now be dealt with by the plugin itself without needing a different interface.

Another change I'd like to make is to store the icons as PNGs in a separate directory icons/. This will remove the (unreadable) memory blob from the plugins, and instead the icon path is derived from the plugin's name (icons/.png).

Apart from the changes already discussed, by applying this interface, the non-intuitive #include "dependency.c can be removed and handled by the build system. For more information, see the other proposal.

I've succesfully applied the interface changes to the pencil plugin and clmedview in my local repository. Changing the other plugins should be fairly straightforward from there.

[minovations]

Hi Roel,

I think that we should not make it harder as needed. The plugin interface is a great ict-ish peace of art, but functional not working if you want to do 3D/4D operations. I think that it would be nice to just skip a lot of overhead and rearrange them within the main source code.

The main problem is located within the layer where the plugin is integrated. The plugin works with a pixel data set. Pixeldata doesn't know anything about >2D information.

I would propose to integrate the plugins at this point, and if possible extract them on a later timepoint.

[roelj]

I think it is possible to do >2D operations on the data with a plugin (with either the current or the proposed interface) because in PixelData there's a pointer to the Serie, which (as far as I understand this) contains the raw data.

With some changes to the build system (as proposed), one could use libmemory in a plugin (or any other library that is needed to manipulate in >2D). So, I don't think integrating the code will provide any benefit over the dynamically loading approach.

Because of the shared objects, we don't have to deal with hardcoded naming. Each plugin is called using the exact same function name, which is defined by a single interface. Integrating the plugins into the code would in the best case lead to redundant code because you have to deal with the differences in function naming. The easiest way to fix this would be to create a list with Plugin structures all containing function pointers to a specific plugin implementation (the same as with dynamically loaded plugins).

Other than this minor issue of code duplication, when adding new plugins, one would have to modify the code preparing or loading the plugins. In the dynamically loading approach, there is no code change required on the clmedview side to add a plugin to the system.

My current implementation has some advantages to the user as well. I recorded a video with the result of fine-grained control over which features a plugin supports.

What we can see in this video, is that a plugin can not only tell which properties it supports, but also limit the values a user can input to common-sense values. One example of this is the minimum value of 10 on the sobel plugin. At the moment, it doesn't make sense to decrease the value below 10, because the plugin doesn't support it. Another example is the fill plugin, which doesn't support the size property, so the user shouldn't be able to set a value..

Now that such a fine-grained control is possible, we could go for a only-show-what's-configurable strategy, which would allow us to implement completely different properties for each plugin without cluttering the user interface.

Lastly, I don't think the proposal is a lot more complex than the current situation. With the proposed interface, clmedview doesn't have to call plugins in different ways according to their "type". It's just plugin->apply (...) now, and plugin->set_property (...) whenever the user changes a property.

The latest commit in my branch merged your changes with the two proposals:
roelj@e69b0b1

It also enables the zlib compression features in the nifti and dicom loaders automatically along with cleaning up a bunch of compiler warnings.

I'm looking forward to send a pull request.

[roelj]

The "new" approach has been merged into master. We still have to look at how to do >2D manipulation, but that's a separate issue now.