# Unsafe vtkPoints::GetPoint(vtkIdType) method

**URL:** https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005
**Category:** Development
**Created:** [October 28, 2021, 6:47pm UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005 "2021-10-28T18:47:49Z")
**Posts on this page:** 16
**Page:** 1

<div class="post-metadata">

### Author: ![lassoan](https://discourse.vtk.org/user_avatar/discourse.vtk.org/lassoan/32/50_2.png) [@lassoan](https://discourse.vtk.org/u/lassoan)
#### Post date: [October 28, 2021, 6:47pm UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005/1 "2021-10-28T18:47:49Z")

</div>

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](https://vtk.org/doc/nightly/html/classvtkPoints.html#ae6b404b5de945757977017cf136d6b9f) 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](https://discourse.vtk.org/t/upcoming-changes-to-vtkcellarray/2066/9), 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?

---

<div class="post-metadata">

### Author: ![will.schroeder](https://discourse.vtk.org/user_avatar/discourse.vtk.org/will.schroeder/32/233_2.png) [@will.schroeder](https://discourse.vtk.org/u/will.schroeder)
#### Post date: [October 28, 2021, 7:24pm UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005/2 "2021-10-28T19:24:47Z")

</div>

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

---

<div class="post-metadata">

### Author: ![lassoan](https://discourse.vtk.org/user_avatar/discourse.vtk.org/lassoan/32/50_2.png) [@lassoan](https://discourse.vtk.org/u/lassoan)
#### Post date: [October 28, 2021, 8:32pm UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005/3 "2021-10-28T20:32:31Z")

</div>

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:

```cpp
  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.

---

<div class="post-metadata">

### Author: ![will.schroeder](https://discourse.vtk.org/user_avatar/discourse.vtk.org/will.schroeder/32/233_2.png) [@will.schroeder](https://discourse.vtk.org/u/will.schroeder)
#### Post date: [October 29, 2021, 11:44am UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005/4 "2021-10-29T11:44:13Z")

</div>

Well Andras I’m admiring your persistence and desire to improve the system 🙂 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 🙂

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.

---

<div class="post-metadata">

### Author: ![lassoan](https://discourse.vtk.org/user_avatar/discourse.vtk.org/lassoan/32/50_2.png) [@lassoan](https://discourse.vtk.org/u/lassoan)
#### Post date: [October 29, 2021, 12:03pm UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005/5 "2021-10-29T12:03:00Z")

</div>

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?

---

<div class="post-metadata">

### Author: ![will.schroeder](https://discourse.vtk.org/user_avatar/discourse.vtk.org/will.schroeder/32/233_2.png) [@will.schroeder](https://discourse.vtk.org/u/will.schroeder)
#### Post date: [October 29, 2021, 1:46pm UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005/6 "2021-10-29T13:46:06Z")

</div>

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

---

<div class="post-metadata">

### Author: ![lassoan](https://discourse.vtk.org/user_avatar/discourse.vtk.org/lassoan/32/50_2.png) [@lassoan](https://discourse.vtk.org/u/lassoan)
#### Post date: [October 29, 2021, 2:45pm UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005/7 "2021-10-29T14:45:48Z")

</div>

[vtkVector3d](https://github.com/Kitware/VTK/blob/master/Common/DataModel/vtkVector.h#L487) 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).

---

<div class="post-metadata">

### Author: ![cory.quammen](https://discourse.vtk.org/user_avatar/discourse.vtk.org/cory.quammen/32/6751_2.png) [@cory.quammen](https://discourse.vtk.org/u/cory.quammen)
#### Post date: [October 29, 2021, 2:53pm UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005/8 "2021-10-29T14:53:56Z")

</div>

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

---

<div class="post-metadata">

### Author: ![lassoan](https://discourse.vtk.org/user_avatar/discourse.vtk.org/lassoan/32/50_2.png) [@lassoan](https://discourse.vtk.org/u/lassoan)
#### Post date: [October 29, 2021, 3:05pm UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005/9 "2021-10-29T15:05:25Z")

</div>

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

---

<div class="post-metadata">

### Author: ![spyridon97](https://discourse.vtk.org/user_avatar/discourse.vtk.org/spyridon97/32/7069_2.png) [@spyridon97](https://discourse.vtk.org/u/spyridon97)
#### Post date: [November 1, 2021, 12:55pm UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005/10 "2021-11-01T12:55:28Z")

</div>

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

---

<div class="post-metadata">

### Author: ![lassoan](https://discourse.vtk.org/user_avatar/discourse.vtk.org/lassoan/32/50_2.png) [@lassoan](https://discourse.vtk.org/u/lassoan)
#### Post date: [November 2, 2021, 1:22pm UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005/11 "2021-11-02T13:22:07Z")

</div>

> [@spyridon97](#):
>
> I believe that at least all usages of those functions within VTK/ParaView need to be removed, and I could help with that.

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?

---

<div class="post-metadata">

### Author: ![spyridon97](https://discourse.vtk.org/user_avatar/discourse.vtk.org/spyridon97/32/7069_2.png) [@spyridon97](https://discourse.vtk.org/u/spyridon97)
#### Post date: [November 2, 2021, 1:28pm UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005/12 "2021-11-02T13:28:16Z")

</div>

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)

---

<div class="post-metadata">

### Author: ![will.schroeder](https://discourse.vtk.org/user_avatar/discourse.vtk.org/will.schroeder/32/233_2.png) [@will.schroeder](https://discourse.vtk.org/u/will.schroeder)
#### Post date: [November 2, 2021, 1:31pm UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005/13 "2021-11-02T13:31:10Z")

</div>

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

---

<div class="post-metadata">

### Author: ![spyridon97](https://discourse.vtk.org/user_avatar/discourse.vtk.org/spyridon97/32/7069_2.png) [@spyridon97](https://discourse.vtk.org/u/spyridon97)
#### Post date: [November 2, 2021, 1:32pm UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005/14 "2021-11-02T13:32:13Z")

</div>

Fixed

---

<div class="post-metadata">

### Author: ![lassoan](https://discourse.vtk.org/user_avatar/discourse.vtk.org/lassoan/32/50_2.png) [@lassoan](https://discourse.vtk.org/u/lassoan)
#### Post date: [November 2, 2021, 1:40pm UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005/15 "2021-11-02T13:40:25Z")

</div>

@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?

---

<div class="post-metadata">

### Author: ![spyridon97](https://discourse.vtk.org/user_avatar/discourse.vtk.org/spyridon97/32/7069_2.png) [@spyridon97](https://discourse.vtk.org/u/spyridon97)
#### Post date: [November 2, 2021, 1:48pm UTC](https://discourse.vtk.org/t/unsafe-vtkpoints-getpoint-vtkidtype-method/7005/16 "2021-11-02T13:48:24Z")

</div>

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