Page MenuHomePhabricator

Display action event handler
Closed, ResolvedPublic

Description

Looking at the broadcast approach in T24216 we need to design the communication between the broadcast class and the listener classes.

Event Timeline

kalali triaged this task as Normal priority.EditedMar 9 2018, 5:12 PM
kalali created this task.

The diagram currently shows a composition approach. This way each developer can easily create its own listener for the display action event broadcast class.
The diagram shows two example listener, that we provide for our scenarios: The first is the classic display interactor, which now listens to and reacts on the action events of the broadcast class. It mimics the behavior of the current DisplayInteractor.

The second example is a custom display interactor, which is used to provide custom behavior. It also listens to the action events of the broadcast class but reacts in a customized manner.
Draft for the custom display interactor:
We create a wrapper object (struct) that can be used to set as an observer for a certain event (the action events coming from the broadcast class). On the event the given std::function will be called.
The function has access to the event itself (as the event contains important information, e.g. the sender).

Such a wrapper object can be created multiple times with different std::functions to listen to different events. Introducing a new event allows to create a new wrapper object to observe the new event.

struct Observer
{
  typedef typename std::function<void(const itk::EventObject& eventObject)> CallbackType;
  CallbackType m_Callback;

  Observer(std::function<void(const itk::EventObject& eventObject)> callback) : m_Callback(callback) {}
  void Callback(const itk::EventObject& eventObject) { if (m_Callback) m_Callback(eventObject); }
};

To create such a wrapper object and connect the callback:

void ConnectDisplayInteractorZoomEvent(std::function<void(const itk::EventObject& eventObject)> callback)
{
  Observer* observer = new Observer(callback);

  typedef typename itk::ReceptorMemberCommand<Observer>::Pointer CommandPointer;
  CommandPointer command = itk::ReceptorMemberCommand<Observer>::New();
  command->SetCallbackFunction(observer, &Observer::Callback);
  m_Observable->AddObserver(DisplayInteractorZoomEvent(nullptr, 0, Point2D()), command);
}

private:
  itk::Object* m_Observable;

Later this can be used as:

mitk::Observer::CallbackType callback = [](const itk::EventObject& displayInteractorEvent) 
{ 
  const auto* zoomEvent = dynamic_cast<const mitk::DisplayInteractorZoomEvent*>(&displayInteractorEvent);
  assert(nullptr != zoomEvent);
/* code goes here */
};
ConnectDisplayInteractorZoomEvent(std::move(callback));
kalali added a comment.EditedMar 12 2018, 2:43 PM

The same pattern is used in the classic display interactor, but the callback-function of the command is predefined: The interactor-class itself contains a Move-, a Zoom-function etc.
However, since the predefined callback-functions are member functions of the class, we don't need the Observer struct and can simply use this in SetCallbackFunction.

Another idea is to stay with the current concept of interaction communication (see http://docs.mitk.org/nightly/DataInteractionPage.html), which is based on a dispatcher retrieving registered service references of InteractionEventObserver. Here the dispatcher actively notifies all known observer which in turn handle the event that has been sent with the notification.
Instead of using the presented itk-command-observer pattern the microservice-approach could be used.
We need to investigate the pros and cons of this approach. It was driven by the idea that our event broadcast class has a similar communication with its listener classes like the InteractionEventObserver with the renderer in the way that one could see our display action events as some kind of interaction event.

Using the microservice-approach (as in InteractionEventObserver) leads to a even more looser coupling: The display handler don't need to know the broadcast class.
The broadcast class simply retrieves all DisplayActionEventHandler that were registered before (e.g. StdDisplayActionEventHandler and a CustomDisplayActionEventHandler). Similar to the dispatcher of the renderer and the broadcast-class itself, that is notified on an interaction event.
The registered handler references will then be notified that a certain display action event should be handled. Similar to the InteractionEventObserver each concrete DisplayActionEventHandler has a HandleEvent-function that will perform the needed action, depending on the received DisplayActionEvent.

void mitk::CustomDisplayActionEventHandler::Notify(const DisplayActionEvent* displayActionEvent)
{
    HandleEvent(displayActionEvent);
}

Similar to the observer / command-pattern from above our custom display action event handler will keep a list of registered objects that correspond to a display action event and contain a std::function. If a received event is the same as the object's event (e.g. by using the event's CheckEvent-function) we call the corresponding std::function.

void mitk::CustomDisplayActionEventHandler::HandleEvent(const DisplayActionEvent* displayActionEvent)
{
  std::list<Observer*>::iterator i = m_Observers.begin();
  while (i != m_Observers.end())
  {
    const Observer* o = *i;

    if (o->CheckEvent(displayActionEvent))
    {
        // execute callback if the received event fits the event of the observer
        o->Callback(displayActionEvent);
     }

    ++i;
  }
  return;
}

What are the (dis-)advantages of this approach?

  • we don't need to have the broadcast-class and the concrete handler-class at one location
  • we stick with the micro service approach that is already used in the level above (between the renderer (dispatcher) and the InteractionEventObserver
  • we have several broadcast classes that will be notified by an interaction event. Now each of this broadcast class will notify each concrete display action event handler. This will lead to more (possibly unnecessary) function calls
    • this depends on the filtering technique used

We can use the previous idea of an observer-list also in the composition approach (in contrast to the microservice-approach): So instead of using the itk::command pattern to create several commands, one for each callback / std::function

Observer* observer = new Observer(callback);
command->SetCallbackFunction(observer, &Observer::Callback);
m_Observable->AddObserver(DisplayInteractorZoomEvent(nullptr, 0, Point2D()), command);

we could simply do once

command->SetCallbackFunction(this, &CustomDisplayActionEventHandler::HandleEvent);
m_Observable->AddObserver(DisplayActionEvent(), command);

This would set the HandleEvent-function of the CustomDisplayActionEventHandler-class as the only callback for all DisplayActionEvent.

The HandleEvent-function would then look like above, where the list of known observer is iterated to find all observer that are valid for the specific event.

An example implementation can be seen here:

TODO

This is clearer than the initial approach using the ConnectDisplayInteractorZoomEvent (as an example for the zoom action). In the end we are moving the observer-list and its iteration from itkObject to our own class:
in itkObject.cxx:

template<typename TObject>
void SubjectImplementation::InvokeEventRecursion(const EventObject &event, TObject *self, std::list<Observer*>::reverse_iterator &i)
{
  // [...]
  while (i != m_Observers.rend())
  {

    // save observer
    const Observer *o = *i;

    if (o->m_Event->CheckEvent(&event))
    {
      InvokeEventRecursion(event, self, ++i);

      if (!m_ListModified || std::find(m_Observers.begin(), m_Observers.end(), o) != m_Observers.end())
      {
        o->m_Command->Execute(self, event);
      }
      return;
    }
    ++i;
  }
  return;
}
floca added a comment.Mar 19 2018, 7:59 PM

Regarding your last post: I Do not understand why we cannot use the itk object infrastructure. Maybe be I missed something, but it seems unnecessary to me to clone it.

We are using the itk object infrastructure by setting the HandleEvent-function as the callback for the observing command. However, in contrast to the original approach we only have this single command as an observer. Before that we had a command for each std::function that we wanted to be called on an incoming action event.
All these commands are stored in the observer-list of the itk object. But we still have to store the std::function so we have to keep two lists.

Having only a single entry-point is basically the same as in the micro-service approach where we have single function (notify) that is called and which then decides what to do.
I think it is easier to manage, if we simply have a class that has a list of observer, where each observer has an event type, an std::function as a filter and an std::function as an action.

Additionally I will look into the idea of providing our own command, that can handle std:.functions.

floca added a comment.Mar 20 2018, 1:01 PM

I think this is partially already to much into implementation/internals and details, while there the design for the interface and the handling to the rest of the world are not specified or design descisions are lacking.

e.g.: Do we want to allow reflection about the active handler mechanisms and how is it covered?

The first question should be: What is the responsibility of the handler/why is he here? How is he used? What features are possible by that or explicitly ignored.

Having only a single entry-point is basically the same as in the micro-service approach where we have single function (notify) that is called and which then decides what to do.

Correct. But also in this situation I want a clear reason why duplicate functionality, that a could reuse. And(!) is not just a trival iteration over a list, if you look at the mechanisms and safe guards implemented in itk::Object to ensure thread save (but non blocking) work on the observer list.

e.g.: Do we want to allow reflection about the active handler mechanisms and how is it covered?

How can we do this? If we go with the std::function approach, which allows to perform any arbitrary action, how can we say something about the result of this action?
E.g. if we provide an std::function that should be called on a DisplayZoomEvent:

std::function<void(const itk::EventObject& eventObject)> callback = [&renderWindow](const itk::EventObject& displayActionEvent)
{
  const auto* zoomEvent = dynamic_cast<const mitk::DisplayZoomEvent*>(&displayActionEvent);
  if (nullptr != zoomEvent)
  {
    // given event is a display zoom event; get the zoom factor
    float zoomFactor = zoomEvent->GetZoomFactor();

    // get all known render windows and change their zoom
    auto renWindows = mitk::RenderingManager::GetInstance()->GetAllRegisteredRenderWindows()
    for (auto renWin : renWindows)
    {
      if (BaseRenderer::GetInstance(renWin)->GetMapperID() == BaseRenderer::Standard2D && renWin != renderWindow)
      {
        BaseRenderer::GetInstance(renWin)->GetCameraController()->Zoom(zoomFactor, zoomEvent->GetStartCoordinate());
        // change zoom event for the next render window
        zoomFactor *= 2;
      }
    }
  }
};

We are changing the zoom for each render window differently and we are only processing 2D-render windows. How should we reflect this?

floca added a comment.Mar 20 2018, 3:50 PM

We are changing the zoom for each render window differently and we are only processing 2D-render windows. How should we reflect this?

Cave: I never said we should do this. I only said we should clarify if

  1. it is in the scope of the handler
  2. If no: if we want to allow it (theoretically) and how we would propose to do it.

So it is totaly fine for me, if we say we don't implement it. But if we want to allow it the handler should be no obstacel.

How could we do it? A low-level design (even with std:functions) would be:

  • If we register the lambdas, we provide an ID that identifies the eventhandling uniquely. This can be also used to remove it (and therefore deactivate it) from the handler.
  • Any reflection must then be implemented on a higher level and is coupled to the handler-level by the IDs. On the higher level you may use class/inheritance/abstract interfaces/hard coded rules/what ever you want.

Hope it is cleare now, in what direction I was thinking.

We are currently using a command-approach (using the itk object infrastructure) to connect std::functions that should be executed on a specific event. The events are used to define which command should be used (execute its std::functions). The events are invoked by the DisplayActionEventBroadcast-class to inform the observer about a certain event.
The DisplayActionEventHandler-class can be used to set a customized command as an observer of a display action event. It provides a general function to connect any display action event with any std::function. The customized command can contain a filter function and an action function which is used to check a condition and execute an action if the event was received.
Using this command the mentioned observer struct is not needed, since the command IS the observer itself.
The DisplayActionEventHandler-class keeps a list of command tags to identify the observing commands.

The implementation is contained in T23760-Custom-multi-widget-editor

kalali closed this task as Resolved.Apr 17 2018, 3:18 PM