Pull request: https://github.com/MITK/MITK/pull/32

The SliceNavigationController class has many methods which assume that the m_CreatedWorldGeometry member is non-NULL which is only the case if Update() has been called previously.

So while the semantics of the class require a call to Update() for proper initialization, the methods must/should still check for a valid m_CreatedWorldGeometry pointer to avoid memory access violations.

The attached pull request adds such a check for the ReorientSlices(...) method. However, there are more un-checked m_CreatedWorldGeometry accesses which would need to be fixed.

A more central question is: Should we just call Update() in the constructor of the SliceNavigationController class to ensure proper initialization?

I see several options here:

1. Add if(...) statements to all functions which access pointers to check for NULL. This might lead to weird semantics, e.g. if the user wants to set the position to a specific slice and he does not get feedback that the position was actually not changed (because some member variable is NULL).

2. Throw an exception in every method if a member variable is not initialized. This gives feedback if the SliceNavigationController is in an invalid state. It will change the semantics of existing methods which already check for NULL pointers.

3. Call Update() in the SliceNavigationController constructor to ensure the validity of the object.

I would prefer option 3, but I am not sure about possible side-effects.

@Daniel: Can you think of reasons not to go for option 3?

(In reply to Sascha Zelzer from comment #3)

I see several options here:

1. Add if(...) statements to all functions which access pointers to check

for NULL. This might lead to weird semantics, e.g. if the user wants to set
the position to a specific slice and he does not get feedback that the
position was actually not changed (because some member variable is NULL).

2. Throw an exception in every method if a member variable is not

initialized. This gives feedback if the SliceNavigationController is in an
invalid state. It will change the semantics of existing methods which
already check for NULL pointers.

3. Call Update() in the SliceNavigationController constructor to ensure the

validity of the object.

I would prefer option 3, but I am not sure about possible side-effects.

Option 3 sounds like the best but it would not work: Update() would not initialize much because m_InputWorldGeometry is not defined yet!

From quickly browsing the file it seems to me that most methods are already implemented in way that you describe as option 1. I could only find two exceptions: AdjustSliceStepperRange(), ExecuteOperation().

I would prefer to add checks to the two mentioned methods (and perhaps check the rest of the code).

Sascha, I understand your "weird semantics" point, but I think that what is being checked is not just "some member" but the primary output of the class.
It does not make much sense to change the output before it has been created. The application should probably check if it had ever received a geometry from this class before requesting a change.

Current release is finished. Reseting target milestone...

Resetting release_fix flag for open bugs which are not yet triaged for the 2014.03 release.

[99d6ca]: Merge branch 'bug-16066-slicenavigationcontrolloer-check-createdworldg

Merged commits:

2014-09-12 02:11:55 Sascha Zelzer [56f2de]
Added more NULL checks for m_CreatedWorldGeometry.

2014-09-12 01:07:12 Sascha Zelzer [798fb7]
Merge remote-tracking branch 'nocnokneo/bug-16066-SliceNavigationController-avoid-CreatedWorldGeometry-NPE' into bug-16066-slicenavigationcontrolloer-check-createdworldgeometry-for-null

2013-09-11 19:59:20 Taylor Braun-Jones [52f5b7]
Avoid NPE when m_CreatedWorldGeometry is NULL