vtkFSLookupTable::MapValue signature changed

Development
#1

I get this compile error with hash xxx:
55>c:\dev\slicer\libs\freesurfer\vtkFSLookupTable.h(84): error C2555: 'vtkFSLookupTable::MapValue': overriding virtual function return type differs and is not covariant from 'vtkLookupTable::MapValue' [C:\Dev\S3\Slicer-build\Libs\FreeSurfer\FreeSurfer.vcxproj]

The reason seems to be change of the MapValue method in VTK to return const char * (it was previously without const).

If we are trying to support two versions of VTK, we might need to wrap this definition in an #if VTK9 block or something similar. If not, just adding const will do.

This was introduced in 47e5662bf321acb87f15e83d7f3415faeb44f72f (ENH: Update VTK)

#2

Yes, the best would be to keep compatibility with both VTK versions using #ifdef. Just send a pull request and we’ll review and merge.

#3

Commit ce5575e36aef609c125b8c3a577a9cccd9cdde30 by Sean McBride “Made vtkScalarsToColor::MapValue() return const” caused this. I think that change could simply be reverted, as it causes a divergence from mainline VTK. The reason for this change was not given in the commit message.

Edit: this is in Slicer’s fork of VTK.

#4

Or we could simply apply the same change on our fork of VTK7, that way after transitioning to VTK9 … the Slicer code base would work without any change.

#5

If making return value const is the right thing to do, why not make a PR to mainstream VTK too? If it is not, we could apply this patch to VTK7 branch too.

#6

As you pointed out, the change is already part of mainstream VTK.

the issue is that currently Slicer can be built with version of VTKs with and without this change.

I propose to update the “older” version of VTK used in Slicer to include the change

#7

This change is not part of mainline VTK, that’s why I was suggesting its reversion.

Edit: it is, I was accidentally looking at an older master of VTK.

#8

Looking at the change in the master branch of VTK, it looks like the const correctness change is available. See https://github.com/Kitware/VTK/blame/master/Common/Core/vtkLookupTable.cxx#L740

If we backport the commit onto the older fork of VTK based of VTK7. See https://github.com/Slicer/VTK/tree/slicer-v7.1.0-2016-09-23-3b13ad9

We wouldn’t have to make the code in Slicer more complex

1 Like
#9

Commit was backported to the Slicer fork of VTK7.1, and Slicer code was updated to word with the const signature.

Additionally, the migration guide was updated. See https://www.slicer.org/wiki/Documentation/Nightly/Developers/Tutorials/MigrationGuide/VTK7-Qt4-to-VTK9-Qt5#VTK9:_Signature_of_vtkFSLookupTable::MapValue_updated

Fixed in r26708