Changing vtkAlgorithm::Update() return type from `void` to `bool`

Development
1

vtkAlgorithm::Update() have been returning void for a very long time and it has bothered me as soon as I started working with VTK.

It means we cannot do this:

if (!filter->Update())
{
  // error processing
}

But instead must do:

if (!filter->GetExecutive()->Update())
{
  // error processing
}

Which looks hacky.

Looking into vtkAlgorithm, it would actually be very easy to forward a boolean, it just was not be done before.

However, making this change properly has some impacts and I want to discuss it with the community before hand. we can either:

1. Deprecate it properly and create a new method that returns bool

Indeed, we cant deprecate without renamming when changing a return type.

This is the “proper” way to do it however it will be VERY impactful as everyone is using Update to update their filter will need to switch to a new method (UpdateWithReturn), it also looks ugly to call another method.

This will impact thousands of VTK users.

  1. “Just” change the return type and break the API

As we go from void to bool, it means that users are NOT impacted, because current usage will still work without issue.

However, VTK developers that inherits vtkAlgorithm and override Update will obviously not be able to build their code, with a very simple fix to adapt the signature of the method.

In order to get a feeling of the number of usages, I created a MR and I found a grand total of 3 overrides in VTK. This will impact a a subset of users with probable enough expertise to fix it swiftly.

  1. Not do anything and keep using GetExecutive

As you can already probably tell I’m in favor of 2, but very open to hear arguments for any of the proposed solutions.

5 Likes
2

I think that Option 2 is the simplest and best approach.

Alternatively you could name the function vtkAlgorithm::UpdateVerify() this still has the imperative “Update” with the added imperative “Verify” in the name.
If this is done I would edit the description in Update() to:
“Bring this algorithm’s outputs up-to-date. For new/upgraded code, use UpdateVerify() since the validity of the update can be easily checked there.”

3

Could you also check other open source projects that use VTK? For example, some that I know of: ParaView, ITK, horos, 3DSlicer.

Maybe there are ways to get a bigger list too, for example looking at the FreeBSD port of VTK, we see a list of other projects that depend on it: FreshPorts -- math/vtk9: Visualization toolkit

4

I’m pretty sure that there will be no issues with the VTK examples. So Option 2 is OK for them.

5

I can look at ParaView but I will let other project run their own check before answering here to give insights.

FreeBSD port of VTK,

It’s a fork, they have other issues then grabbing a commit from master.

6

The override keyword on any overridden Update() function should point out the change in dependent projects, and if they don’t have override, there will be a warning about shadowing I believe.

1 Like
7

Not sure what you mean… I was pointing it out only as an example of how to find other open source code that uses VTK. At the given link there’s a section “This port is required by” that lists various other libs/apps that use VTK.

8

Ok, got it.

So I just built ParaView with my test branch locally and in CI and found a single use case needing an update, which is very reasonnable.

9

I’m fine with option (2). I found just two classes needing updates in vtk-dicom.

When the return type changes, it would be nice to add a feature test macro in vtkAlgorithm.h:

#define VTK_ALGORITHM_UPDATE_RETURNS_BOOL 1
10

It seems pretty clear that Option 2 will bring the most benefit with the least downside

1 Like