Page MenuHomePhabricator

Enforce atomic state transitions in MITK Core classes
Closed, ResolvedPublic

Description

I discovered a few sever problems in the MITK Core while I was testing our custom viewer based on QmitkStdMultiWidget. The problems affect the DisplayGeometry and SliceNavigationController and maybe the BaseRenderer as well.

Basically, the problem is that many unnecessary events are sent about a state change. The SliceNavigationController has a private BlockUpdate function to handle this, but the DisplayGeometry does not.

For instance, signals are sent out while the state of an object is just temporary, i.e. it is not the final state that the object should reach. E.g. the following function will send a few itk::ModifiedEvent() from both function calls, resulting in 3-5 ModifiedEvents. Only one signal should be sent at the end of the function.

bool mitk::DisplayGeometry::ZoomWithFixedWorldCoordinates(ScalarType factor, const Point2D& focusDisplayUnits, const Point2D& focusUnitsInMM )

{

assert(factor > 0);

SetScaleFactor(m_ScaleFactorMMPerDisplayUnit/factor);
SetOriginInMM(focusUnitsInMM.GetVectorFromOrigin()-focusDisplayUnits.GetVectorFromOrigin()*m_ScaleFactorMMPerDisplayUnit);
return true;

}

This has a few major consequences. It can happen that the renderer is updated while the display geometry is in a temporary state showing some invalid image on the screen. Moreover, if the renderer update is triggered by the display geometry modifications, the rendering can become slow or flickering because of the unnecessary updates.

But what causes even more troubles for me at the moment is that interacting with the display geometry can be difficult. We have three different kinds of viewers (one in an editor, two in different plugins) and they should interact with each other in various ways. E.g. the orientation, selected position and zooming are synchronised between them. They communicate through DisplayGeometry, SliceNavigationController and FocusManager events. Because of the unwanted signals they need to react on events that they should not need to, and sometimes the state of these objects (DG, SNC, FM) is inconsistent (maybe not individually, but compared to each other).

A typical example is that if you click in a render window then (up to) 2 GeometrySliceEvents are sent, what is correct, but the first signal is sent out early, before the other SNC is updated. And so, if someone is listening to the SNC events, he will see the SNCs in a state that the user did not want.

For our viewer I follow a relatively simple design pattern to ensure that all the state transitions are atomic. I also wrote a simple test framework for writing unit tests to enforce this requirement. It can automatically check if:

  • a signal is sent only if the state has changed
  • the state does not change twice (after a signal has been sent)
  • the new state is consistent
  • the new state is equal to the expected state (if the expected state was set beforehand).

I think it would be worth enforcing these constraints on the MITK Core classes. I would be happy to provide the my generic tester and the design pattern.

Event Timeline

I shared the test framework with a little unit test.

Pull request sent:

https://github.com/MITK/MITK/pull/45

Branch:
bug-17295-atomic-state-transitions

The test checks the display geometry modifications after a window resize. There should be only 4 itk::ModifiedEvents sent out (one for each window), but there are 25 (!).

There are some problems with the test. One is that the loaded image does not appear in the windows for some reason. The second is that the test crashes at exit, some VTK resource is not released.

The branch is forked from a recent commit of the MITK master, but not the latest because that did not build. (I had to disable two modules also for this because there is no Qt5 on my system.)

The pattern that worked for me:

  1. The object has data members to represent its full state.
  1. The object has bool data members (flags) to remember which part of the state has changed. (Alternatively, an another set of the previous data members.)

public:

/// Returns true if the update was already blocked.
/// Calls Update() when unblocked.
bool BlockUpdate(bool blocked);

private:

/// Updates the render windows, SNCs, focus manager etc.
/// according to the state changes.
/// Clears the state change flags.
/// Sends out the signals about the changes.
void Update();
  1. Any public function or interaction must start and end with:

{

bool updateWasBlocked = this->BlockUpdate(true);
...
this->BlockUpdate(updateWasBlocked);

}

  1. Optional:

public:

/// Returns true if the signals were already blocked.
bool BlockSignals(bool blocked);
kislinsk claimed this task.
kislinsk added a subscriber: kislinsk.

Meanwhile, the mitk::DisplayGeometry was removed.