Adding and wrapping a custom namespace within VTK

I’m currently working on adding orientation support to vtkImageData (more on Discourse and GitLab).

While working on IO, I came up with this solution to define anatomical orientation, axis, to be able to get the transform from an anatomical coordinate system (whatever the file we read holds) to another (LPS for VTK): https://gist.github.com/agirault/5efa4188be4af41e0f09311cd325b786

I can reorient a direction matrix that was in a different coordinate system than LPS with those couple lines:

  // Parse values from file
  double direction[9] = ...; // 3x3 direction matrix
  const char *acronym = ...; // anatomical orientation acronym. Ex: RAS
  vtkAnatomicalOrientation::CoordinateSystem fileOrientation(acronym);

  // Adjust direction matrix if not in LPS
  if (fileOrientation != vtkAnatomicalOrientation::LPS)
  {
    double orientation[9]={0.0};
    fileOrientation.GetTransformTo(vtkAnatomicalOrientation::LPS, orientation);
    vtkMatrix3x3::Multiply3x3(direction, orientation, direction);
  }

But since it is composed of static functions, enums, and classes within a namespace, it’s having a hard time “Generating the wrap hierarchy”.

How would you suggest I integrate this in VTK’s build architecture? Can I stick to using a namespace? How about the wrapping?

Thank you

Paging @dgobbi.

Instead of a namespace, can you put them in a class that is nothing but static methods instead? Those should get wrapped.

Yes I could make vtkAnatomicalOrientation a class instead, if it’s fine to embed other classes within? Do we have existing classes which only have static methods so I can check how they work? I want to make sure people don’t instantiate it.

Another solution I thought of was to embed those static methods and the Axis direction in CoordinateSystem (which I would rename something like vtkAnatomicalSpace instead), but then I don’t know how I can declare LPS

As Ben said, the best approach for wrapping is to make a class with static methods. Examples of this are vtkMath and vtkStructuredData.

The namespace wrapping in VTK is limited. Namespaces can only contain constants, nothing else.

Classes that are in namespaces (or in other classes) are not wrapped.

Yikes. Not going to cut it with my current infrastructure… I’ll think of a “VTK” way to adjust it, but if there is interest in thinking about wrapping of namespaces and embedded classes, I’m all hears (cc: @cory.quammen)

Note: to make it impossible to instantiate a class,

  • declare a protected/private constructor (as most VTK classes do)
  • do not declare a New() method
1 Like

I can also give a summary of why the wrappers don’t support namespaces yet.

When the wrapper front end parses the C++ declarations and builds the tree, the tree is complete, and includes all embedded namespaces and classes. So far so good. But the back end only generates Python wrapper code one level deep. Why is this? The reason is that the rules for identifier lookups in C++ are quite intricate. Wrapping a class requires identifying all the types, constants, etc. that its API uses. Given embedded namespaces, superclasses, and scope operators, this is very difficult to implement. I worked on it for a while back in 2012 or so, but stopped when it became clear that time could be better spent on other wrapper features (e.g. support for templated classes).

Noted, thanks for the explanation @dgobbi. That’s too bad because it’s going to make the C++ interface more complicated… but it should mainly be used by devs and not users, so I guess that will have to do.

Other question: could I still use a namespace but “manually” wrap it?

Rather than manually wrapping, it would be much easier write a pure-python implementation that does exactly the same thing as the C++ code.

But I strongly recommend that you redesign the interface instead. You could put everything into a vtkAnatomicalCoordinateSystem class, for instance. I don’t see a compelling reason for vtkAnatomicalOrientation and vtkAnatomicalOrientation::CoordinateSystem to be separate entities.

Agreed, I actually mentionned that above. The only issue I then see is that I can’t define LPS anymore.

Also, I suppose I won’t be able to use custom constructors either?

You can use custom constructors, the Python wrappers handle them just fine. If you want to use them, then just don’t derive your class from vtkObject. The restriction against custom constructors has everything to do with vtkObject::New() and nothing to do with wrapping.

Edit: vtkVariant is a good example of a class that has many custom constructors.

1 Like

Perfect thanks, I’ll try that out and keep you updated. Didn’t know if it was ok to go with custom constructors and bypass New(). That also means not compatible with smartpointers?

Your class will be very lightweight, so I don’t think the use of pointers with it would be of much benefit anyway. Just use pass-by-value or pass-by-reference, e.g. use it the same way that std::string or vtkVariant are typically used in VTK interfaces.

By the way, your interface uses parameters of type “const std::string” but that makes no sense and must be a mistake… did you mean to use “const std::string&”?

1 Like

Sounds good, and yes :slight_smile:

Enum classes aren’t fully wrapped yet, but they will be by next week (see !5614).

1 Like

That’s exciting! Will this handle enums and enum classes embedded in another class? That’s the only thing that is failing for me right now here, with those errors:

  • enum: vtkAnatomicalOrientationPython.cxx:444:22: error: use of undeclared identifier 'Axis'; did you mean 'vtkAnatomicalOrientation::Axis'?
  • enum class: vtkAnatomicalOrientationPython.cxx:118:16: error: no matching function for call to 'PyvtkAnatomicalOrientation_Axis_FromEnum'

Apart from that, it looks pretty good, thanks for the help:
https://gitlab.kitware.com/alexis-girault/vtk/commit/4df1ce0e8ddbd182f91cef4528e5b3efab7ea52a

Yes, the lack of proper enum class support is why you are seeing those wrapping errors (the wrappers aren’t properly scoping the enum members).

The implementation was made possible thanks to the following PRs from @dgobbi, and !5614 and !5647. Thank you!