vtkFSLookupTable::MapValue signature changed

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)

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.

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.

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.

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.

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

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.

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

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