Page MenuHomePhabricator

The application crashes if you load a plane object/surface
Closed, ResolvedPublic

Description

The application crashes if you load (or somehow create) a plane object/surface.
(crash in Debug mode, VTK Warnings in Release mode).

For instance use the segmentation bundle to create a polygon model with just one slice. Save the model, restart MITK and load it again.

Stack trace:
void PlaneGeometry::SetBounds(const BoundingBox::BoundsArrayType &bounds)
...
assert(bounds[3]>0);

Event Timeline

I discussed this with Casper and Jan. We assume that during the loading process a 3D object is generated and by definition a 3D object needs 3D bounds. That means bounds[3] > 0 has to be correct.

Possible solutions are:

  1. A 3D object could be 2D object. Meaning one of the 3 dimensions is allowed to be 0.
  2. Check the dimensions of the unkown object before, and generate a 2D object instead, if one dimension equals 0.

> Request for discussion.

This bug is also present in 3M3. No crash happens but a lot of warnings occur.

This was discussed shortly at the MITK meeting, but without a decision. Ivo, do you have an idea/preference?

(In reply to comment #2)

I discussed this with Casper and Jan. We assume that during the loading process
a 3D object is generated and by definition a 3D object needs 3D bounds. That
means bounds[3] > 0 has to be correct.

ASAIK, the bounds array holds start-end value-pairs, thus bounds[3]>0 means that the extension of the SECOND dimensions (not the third!) has been > 0. This also matches the comment two lines above the assert statement ("//the unit rectangle must be two-dimensional"). It seems that this makes sense for a PlaneGeometry - otherwise it is a line, not a plane ...
There is no check for the third dimension (bounds[4] and bounds[5]), thus a plane with zero thickness seems to be ok.

Therefore, the actual question might be: why is the second dimension zero? Maybe because the PlaneGeometry is initialized by a reinit-process of the three orthogonal views using an object with zero thickness, which is - orthogonal to this - a line.
If this is indeed the reason, then a pragmatic fix may be to set dimensions of 0 in the reinit-process (not in PlaneGeometry!!) to 1/1000 of the minimal, non-zero dimension and to 1, in case all dimensions are 0.

In
void PlaneGeometry::InitializeStandardPlane( const Geometry3D *geometry3D,

PlaneOrientation planeorientation, ScalarType zPosition, 
bool frontside, bool rotated )

added output

std::cout << "planeorientation - " << planeorientation << std::endl;
std::cout << "0 - " << geometry3D->GetExtent(0) << std::endl;
std::cout << "1 - " << geometry3D->GetExtent(1) << std::endl;
std::cout << "2 - " << geometry3D->GetExtent(2) << std::endl;

resulted in

planeorientation - 0
0 - 54.00
1 - 0.00
2 - 33.00

using the attached surface, whereas width and height are defined by

width = geometry3D->GetExtent(0);
height = geometry3D->GetExtent(1);

Will try to create surfaces oriented in another direction to check whether plane orientation is correctly set.

void PlaneGeometry::SetBounds(const BoundingBox::BoundsArrayType &bounds)

is called several times (once with each planeorientation and then again twice with orientation transversal) each time a global reinit is done.

As the asserts fail for each orientation but one for a two dimensional surface the application crashes.

Setting the bounds to a minimum value greater than zero prevents the crash but still results in a vtk Error window
vtkTransformPolyDataFilter : No transform defined
The surface is displayed in the 3D and the two orthogonal 2D Windows, but not in the parallel 2D window. Could this be related to the 0.5 offset bug?

Added a check in the mitkRenderingManager which checks whether any of the bounds of the Geometry3D are closer together than an epsilon. If that is the case one is added to the upper bound.

A test should be added checking for this crash and I am not certain, whether it could produce some unwanted effects for e.g. very small images

New Patch including test

Thanks for working on this and also for implementing a test case prior to the change request! Unfortunately, I have problems understanding your solution:

  • I don't see a direct answer to Ivo's question about the origins of the crash
  • the change request states that PlaneGeometry::SetBounds() has an assertion which fails. However, your suggested change is in RenderingManager.
  • your code changes around the for-loop in RenderingManager for( unsigned int dimension = 0; ( 2 * dimension ) < newBounds.Size() ... is a little hard to understand. You could avoid 2*dimension+1 by using clearly named variables. Also, I don't see the reason why you copy geometry instead of changing it.

Maybe your change is perfect and just needs a little more explanation.

  • I don't see a direct answer to Ivo's question about the origins of the crash

    His suspicions mentioned in comment #5 seem to be correct.
  • the change request states that PlaneGeometry::SetBounds() has an assertion which fails. However, your suggested change is in RenderingManager.

The failing assert in the SetBounds() is the direct cause of the crash, but just commenting out the assert results in vtkOutput errors. As mentioned by Ivo in comment #5 fixing the bounds in the PlaneGeometry does not work, but they have to be adjusted prior to that in the initialization process.

  • your code changes around the for-loop in RenderingManager for( unsigned int dimension = 0; ( 2 * dimension ) < newBounds.Size() ... is a little hard to understand. You could avoid 2*dimension+1 by using clearly named variables. Also, I don't see the reason why you copy geometry instead of changing it.

The reason for the copy instead of the change is, that the bounds are part of a const within the geometry.
The (2 * dimension + 1) juggling is the same as some lines further up in the code, as each bound is saved as two values, start and end, as mentioned by Ivo.

The fix should be tested using very small surfaces for different behaviour.

Ivo, do you agree with the proposed fix?

[SVN revision 28737]
FIX (#4598): Setting zero image extent of the bounding box to one to prevent crash

That commit was by me, it seems Marco didn't switch the account back after commiting...

kislinsk merged a task: Restricted Maniphest Task.Aug 1 2016, 10:47 PM