VtkCellLocator and Java

I’ve noticed that several of the methods in vtkCellLocator (as well as some other classes) don’t come over to Java. Are there any plans to improve this in the future?

Yes but only if we are aware of them so we can make sure they get properly wrapped. Could you specify the set of methods that you’ve noticed that are missing?

@dgobbi may have some idea on what could be going on.

Here’s the ones we’ve patched in the past to be exposed, which has worked, but doing the same with vtk8 builds seems to (maybe?) cause some other rendering problems (I don’t see how they are connected). It would be nice though if the following were officially exposed to Java and tested by the Kitware folks.

Common/DataModel/vtkCellLocator:

  • IntersectWithLine
  • FindClosestPoint
  • FindClosestPointWithinRadius
  • FindCell
  • FindCellWithinBounds

Common/DataModel/vtkTriangle:

  • IntersectWithLine

Filters/FlowPaths/vtkModifiedBSPTree:

  • IntersectWithLine
  • FindCell

Filters/General/vtkCellTreeLocator:

  • IntersectWithLine
  • FindClosestPoint
  • FindClosestPointWithRadius
  • FindCell
  • FindCellWithinBounds

These were all needed because some of the arguments are passed in by reference in C++ (typically arrays), which I guess the make system doesn’t properly translate to Java?

Just out of curiosity what kind of patching did you do to expose those methods? Just an example might be useful.

Getting pass-by-reference to work with the Java wrappers is a non-trivial modification. If you have code that does this, then by all means share that code, even if it doesn’t work with the latest version of VTK.

For the Python wrappers, pass-by-reference is handled via the special PyVTKReference object and (for arrays) via the sequence protocol.

I’d like to come back to this later on - I have a more pressing problem that I’ll bring up in another topic.

OK, finally, FINALLY, coming back to this. What we’ve done is made child classes of the classes I mentioned above (since we can’t declare the method twice in the same file), and for the methods I mentioned, changing any places where an array is mentioned (which in C could be any size) and changing the signature to a fixed size array - this seems to help the wrapping mechanism recognize it as a valid function that can be exposed to Java (maybe? I’m unclear exactly how the wrapping works). This technique was done before I started working on this project, so I’m not sure if there is another way to do it that is more efficient. Here’s an example: For Common/DataModel/vtkCellLocator, we have a substitute class, vtksbCellLocator, that does the following for FindCellsWithinBounds (whose signature in the parent class is FindCellsWithinBounds(double*, vtkIdList*)

  // Description:
  // This simply calls the corresponding method in the base class but
  // allows this function to be called from Java since all pointers
  // have been replaced with 1 element arrays.
  void FindCellsWithinBounds(double bbox[6], vtkIdList *cells)
  {
	  return Superclass::FindCellsWithinBounds(bbox, cells);
  }

That’s a representative example of what we do in order to expose these methods to Java. So I guess what I’m looking for is:

  • Can a method like this be exposed to Java without jumping through these hoops? From your previous responses, it sounds like the answer to that is “no” even though it is possible in Python
  • If not, what is the possibility of adding a similarly named method that adopts this signature so it can co-exist alongside the current method call? The compiler (obviously) sees the signatures as the same, so it says it is a redeclaration of the method. Since it will be named differently, then maybe it would be exposed to Java?

We’re trying to avoid having to patch the VTK code every time a new release comes out and build it ourselves, and having this built in would save us a lot of time!

Any information or guidance you can give would be appreciated. I’m more than happy to put in an MR once a decision is made.

–Josh

It looks like we should make a PR with the proper known size directly in the VTK classes.

What do you think @ben.boeckel?

For methods like FindCellsWithinBounds() that don’t wrap because they pass a pointer instead of an array, you can submit an MR that changes the parameter to an array. That’s all that’s needed for those ones.

E.g. in vtkCellLocator.cxx:

-  void FindCellsWithinBounds(double* bbox, vtkIdList* cells) override;
+  void FindCellsWithinBounds(double bbox[6], vtkIdList* cells) override;

As for why this method didn’t use an array parameter to begin with, some VTK developers (especially new or temporary ones) prefer pointer parameters over array parameters and are not aware of the impact their choice has on the wrappers. The VTK code review process isn’t perfect and we should be doing better…

Methods like IntersectWithLine() that use pass-by-reference are the ones that are really hard to wrap:

  int IntersectWithLine(const double a0[3], const double a1[3], double tol, double& t, double x[3],
    double pcoords[3], int& subId, vtkIdType& cellId, vtkGenericCell* cell) override;
1 Like

For the ones with pass-by-reference, we replaced those with size-1 arrays, and that seems to do the trick.

int IntersectWithLine(const double a0[3], const double a1[3], double tol, double t[1], double x[3],
    double pcoords[3], int subId[1], vtkIdType cellId[1], vtkGenericCell* cell);

As far as I’m aware, the Java wrappers can’t return the subId or the cellId via the array parameters. So even though this allows the method to be wrapped, it doesn’t provide all of the functionality.

OK interesting - I’ll admit I am not totally intimate with these methods - let me double check and see if we use them (we definitely use some of the ones I mentioned above - I’ll double check that tomorrow and get back to you).

OK from what I can tell: there are several places where we do indeed call IntersectWithLine and FindClosestPoint in our vtkCellLocator child class, and routinely use the returned cellID, which in the original method signature is vtkIdType& cellId but we have it as vtkIdType cellId[1] in our child class. And we’ve been using this for years (well before I started on the project even), so I can only conclude that it works as intended. We don’t seem to use the subId and dist2 values, instead just providing properly sized arrays (both size 1) to have the wrapping system accept them (and checking the values of those in a few places, they look sensical, I think).

We don’t currently use the other classes I listed above, but I believe the original author updated those signatures as well just so we could use them if we wanted to in the future.

Given that - any further thoughts?

If the Java wrappers are already returning values via array parameters, then I can modify them to accept a unit-sized array as a reference parameter. That shouldn’t be too difficult. I’d need someone else to do the testing, since I’m not a Java programmer and even though I’ve written Java tests in the past, I’m neither efficient at doing it nor do I particularly enjoy it.

I’ll post a link to a merge request once I have something ready for testing.

Thanks @dgobbi for trying to improve the java wrapping part. @Gilthalas would you be able to validate it on your end, since you are setup for it? You can use me in your PR to trigger test and merge them if we all think they are good to go.

Thanks both of you for taking that on…

Yeah I can find some time for that - thanks a bunch for making these improvements! This should make working with VTK much easier for us in the future!

I started work on it over the weekend, you can find it here.
I haven’t checked to make sure that it actually works.

Hi - initial looks at this shows that it works - although I’d like to have some of my folks take a closer look.

I think my plan going forward will be to get this MR approved and merged first - since we definitely use the by-reference methods. Once that is done, I’ll do a followup to change some of the signatures that pass a pointer, because while we don’t actively use them, I would like to have them available (since we have had them that way in our fork for years).

I’ll comment back when I have an update.

1 Like