Skip to content

Commit

Permalink
BUG: avoid crash with LONG images
Browse files Browse the repository at this point in the history 
vtkImageInterpolator explicitly disallows reslicing of
data types that cannot be represented by a double, so
Slicer would crash when a volume of this type was loaded.

Thie change prevents the crash and generates an error message.
I'm not sure there's a better solution at this level of the
code, but it would probably make sense for the application
to generate a warning when long types of volumes are added
to the scene.

From: Steve Pieper <pieper@isomics.com>

git-svn-id: http://svn.slicer.org/Slicer4/trunk@28563 3bd1e089-480b-0410-8dfb-8563597acbee
  • Loading branch information
@pieper
pieper committed Oct 21, 2019
1 parent b485c8b commit d596340
Showing 1 changed file with 12 additions and 0 deletions.
12 changes: 12 additions & 0 deletions Libs/MRML/Logic/vtkMRMLSliceLayerLogic.cxx
View file
Expand Up @@ -645,6 +645,18 @@ void vtkMRMLSliceLayerLogic::UpdateImageDisplay()
return;
}

vtkImageData *imageData = this->VolumeNode->GetImageData();
if (imageData != nullptr &&
(imageData->GetScalarType() == VTK_LONG ||
imageData->GetScalarType() == VTK_UNSIGNED_LONG ||
imageData->GetScalarType() == VTK_LONG_LONG ||
imageData->GetScalarType() == VTK_UNSIGNED_LONG_LONG))
{
vtkErrorMacro("Reslicing can only be done on types representable as double. Node " <<

This comment has been minimized.

Sign in to view

Copy link
@lassoan

lassoan Oct 21, 2019

Contributor

This message is good as a source-code comment but as an error message shown to users something like this could work better:

"Image reslicing is not implemented for 'long' voxel types. Node..."

Users would then understand that it could be doable just currently not implemented. Also, it is less confusing if we don't bring up the possibility to convert to double (it is an internal implementation limitation of the image reslice filter).

This comment has been minimized.

Sign in to view

Copy link
@pieper

pieper Oct 21, 2019

Author Member

Hmm, yes, but mentioning the double thing gives them something concrete so they can figure out the solution themselves. Anyone who looks in the log file should be given something that can help, like the criteria for data that is supportable.

This comment has been minimized.

Sign in to view

Copy link
@lassoan

lassoan Oct 21, 2019 •
edited

Contributor

Log file contains filename and line number so developers can find all the exact details there (your comment about double, reslice image filter, etc).

Anyway, I don't mind including details in the log that are not useful for users (we have many of these), but it would be nice to make it more clear that reslicing of these voxel types has simply not been implemented (users would then know that it is not some fundamental flaw of the software but they should just ask for it to be implemented).

Since it is an application-level class, we could even make a suggestion to cast the image and/or add a link to the bugtracker where workaround can be described.

It's up to you, the current behavior is already way better than the unexplained crash before.
this->VolumeNode->GetName() << " has image data of type " << imageData->GetScalarTypeAsString());
return;
}

vtkMTimeType oldReSliceMTime = this->Reslice->GetMTime();
vtkMTimeType oldReSliceUVWMTime = this->ResliceUVW->GetMTime();
vtkMTimeType oldAssign = this->AssignAttributeTensorsToScalars->GetMTime();
Expand Down

3 comments on commit d596340

@pieper
Copy link
Member Author

@pieper pieper commented on d596340 Oct 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue added here to track: https://issues.slicer.org/view.php?id=4714

@lassoan
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These kind of images are so rare that I think this solution should be enough.

@pieper
Copy link
Member Author

@pieper pieper commented on d596340 Oct 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An application level notification about why this data isn't rendered and suggestion about how to workaround would be helpful. But it's not in my critical path.

Please sign in to comment.