Unsafe vtkPoints::GetPoint(vtkIdType) method

Development
1

We ran into an issue due to documented but unexpected behavior of the VTK API. A new developer introduced a bug by calling double* vtkPoints::GetPoint(vtkIdType) multiple times and did not expect that a new call to GetPoint overwrites all previously returned values.

The method documentation correctly describes limitation of the method, so we could say that it’s the developer’s fault of not reading the documentation. However, it would make VTK-based software more robust if it was harder to misuse the API.

What do you think about deprecating/removing the error-prone double* vtkPoints::GetPoint(vtkIdType) method from the VTK public API?

The method is very convenient and safe to use from Python (as the array values are copied anyway), so it should be kept for that purpose (for example, by adding #ifdef __VTK_WRAP__ around it instead of actually removing the code).

The issue is somewhat similar to this, with a difference that this GetPoint API has been around for many years, so it is not a recently introduced change in behavior. Still, maybe a similar approach can be used to address both - maybe using VTK_DEPRECATED macros?

1 Like
2

What do you think about deprecating/removing the error-prone double* vtkPoints::GetPoint(vtkIdType) method from the VTK public API?

This is a mammoth disruption, it would break code all over the place, including external classes and applications.

I agree that this is a holdover from 28 years ago and I don’t like it either, but the effort to clean this up would be too much. Also keep in mind that to do this right we’d have to follow precedent and consider the data arrays, which also have T* GetTuple… type API.

3

The single-argument GetPoint method is not used at all in about 95% of VTK’s cxx files. It would take some effort to review the impacted 200-300 files, but it would be a fairly simple change. External software that wants to keep using the method could use a CMake flag to indicate they need legacy methods.

In VTK, GetPoint is usually used along with a SetPoint or InsertNextPoint (and often accompanied by CopyData) in a seemingly very inefficient way. This is a typical pattern:

  for (vtkIdType ptId = 0; ptId < numPts; ++ptId)
  {
    this->Locator->InsertNextPoint(inPts->GetPoint(ptId));
    newPointData->CopyData(inPointData, ptId, ptId);
  }

It is hard to tell how much improvement could be achieved by simplifying the code (at least replacing the the set/get and insert/get pairs by a copy method), but considering how often GetPoint/SetPoint method comes up during profiling, there is a chance that there could be perceivable performance improvements just by rewriting these inefficient loops.

It would be no small effort overall, but there would be significant benefits (safer API, potentially better performance) and most of the work could be completed by an intern or student completely new to VTK.

For convenient access to point coordinates maybe adding a method like vtkVector3d GetPoint(vtkIdType) could be considered.

4

Well Andras I’m admiring your persistence and desire to improve the system :slight_smile: Let me make a few comments.

To me it’s about cost/benefit analysis: if I had the time it’s going to take to do this, I’d certainly put this lower on the list as an issue to spend time on. To do this right, you really need to consider the effort to update VTK including possibly testing code and documentation (the book(s)); updating major platforms like Slicer and ParaView; dealing with unhappy users and developers; and I would argue also addressing vtkDataArray API this is based on. And at the end you’d keep the method around anyway for Python - which BTW would represent a soft bifurcation between the C++ and Python API. I think it’s easier just to teach someone to read the documentation :slight_smile:

But if the community still thinks this is a high priority, I suggest the long game. Demonstrate commitment to the change process by first working through all of VTK and related code and remove the single-argument GetPointCode(). Once it’s been demonstrated that there is the will to follow through and do this, and no problems crop up, then we can start the legacy process and notify users. My main point is I’d personally like to see follow through and evaluation before committing to this effort - there are too many halfway implemented design patterns/best practices in VTK and I want to avoid adding yet another.

5

I am not sure about the final cost/benefit ratio yet and how it compares to other tasks. But at least it is worth spending a little time with thinking about potential solutions - as we do it in this discussion and maybe a few little experiments and performance profiling.

What do you think about returning vtkVector3d?

6

Makes sense, although I could see a conversation about renaming and generalizing it, maybe vtkTuple<type,dim> etc.

7

vtkVector3d is a vtkVector3<double> that is a vtkVector<T, 3> that is a vtkTuple<T, Size>. Using the vtkVector3d class over a templated type would be mainly a convenience (and maybe easier for the VTK Python wrapper to process).

8

Indeed. It’s unfortunate that “vector” is such an overloaded term. How about a convenience vtkPoint3d object instead of vtkVector3d as a clearer name?

9

Finding a good name is important, but there are a number of higher-level questions to discuss before decisions could be made on names. I’m not sure what developers think about the overall idea.

  • Does returning point coordinates as a fixed array object instead of a double* makes sense vtkPoints::GetPoint(n)?
  • Should we do it when there is a dedicated underlying array (no risk of a subsequent call overwriting the returned value)?
  • Should we do it everywhere where small fixed arrays are returned (2D/3D/4D points and vectors, RGB/RGBA colors, maybe even 6-component extents and bounds)?
10

@lassoan As someone that works on multithreading a lot of different VTK/ParaView filters lately, and calling GetTuple/GetPoint(vtkIdType) functions are not thread-safe, I would really want this to happen.

I have spent multiple times wasting 1-2 days figuring out why the multithreaded code was crashing and the reason end up being that a class used by the multithreaded code was calling those functions.

I was about to initiate the same discussion. Thanks a lot for starting it.
I believe that at least all usages of those functions within VTK/ParaView need to be removed, and I could help with that.

As far as returning coordinates/tuples as a fixed array object instead of a double*, it totally makes sense to me, because it lets you know the size of the used tuple which leads to safe memory access.

1 Like
11

Sounds great! What do you think about adding a CMake build option to disable these unsafe methods (leave it there for Python wrapping only) so that the usages can be detected and removed from VTK (and ParaView, Slicer, …) during test builds?

12

For me this process would have 4 steps:

  1. Remove usages of not thread-safe GetTuple/GetPoint within VTK/ParaView (Difficulty: replacement of functions → easy)

  2. Add functions to return vtkTupleNd objects and deprecate the old ones. (Difficulty: design choices need to be made, and the community needs to be notified because of deprecation → medium)

  3. Add a CMake build option to disable these unsafe methods (Difficulty: the community needs to be notified once more → medium)

  4. Remove these unsafe methods from everywhere, e.g. vtk examples, notify users, etc. (Difficulty: time-consuming → hard)

13

The name vtkVectorNd is problematic, use something like vtkTupleNd - it’s consistent with the DataArray terminology.

14

Fixed

15

@spyridon97 the steps you describe sound good to me. Implementing Step 3 first (disable unsafe API) could help implementing Step 1 (finding and replacing unsafe APIs).

@will.schroeder Would you add vtkTuple3d and deprecate vtkVector3d?

16

@lassoan I think you are right, we could create this cmake flag and have it enabled by default, but while developing, we could disable it to find the unsafe usages.