Adding and wrapping a custom namespace within VTK

proposal
(Alexis Girault) #1

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

(Ben Boeckel) #2

Paging @dgobbi.

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

(Alexis Girault) #3

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

(David Gobbi) #4

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.

(Alexis Girault) #5

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)

(David Gobbi) #6

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
(David Gobbi) #7

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).

(Alexis Girault) #8

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?

(David Gobbi) #9

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.

(Alexis Girault) #10

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?

(David Gobbi) #11

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
(Alexis Girault) #12

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?

(David Gobbi) #13

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
(Alexis Girault) #14

Sounds good, and yes :slight_smile:

(David Gobbi) #15

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

1 Like
(Alexis Girault) #16

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

(David Gobbi) #17

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).

(Alexis Girault) #18

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