Page MenuHomePhabricator

New multi render window widget architecture breaks rendering mode handling
Closed, ResolvedPublic

Description

The rendering mode is ultimately a property of mitk::BaseRenderer, that affects its encapsulated vtkRenderer. A vtkRenderer is added to a vtkRenderWindow and a vtkRenderWindow can manage n vtkRenderers. In the workbench scenario, a mitk::BaseRenderer is deeply nested in a window/widget/class hierarchy and composition like QmitkAbstractMultiWidgetQmitkStdMultiWidgetQmitkRenderWindowWidgetQmitkRenderWindowmitk::RenderWindowmitk::RenderWindowBasemitk::VtkPropRenderer. All or most of these classes take a rendering mode parameter in their constructors to pass them down to the nested class(es). The idea is that a rendering mode can be specifically set for a single renderer, but can also being easily set by an accompanied render window and at higher level even by composite multi render window widgets for all containing renderers at once. There are some flaws to this approach as well as old and new bugs.

A new bug is that the QmitkAbstractMultiWidget-deriving classes do not pass the rendering mode down to their render window widgets. They do store a value copy of the rendering mode, even though it is not a property of themselves.

We are treating the rendering mode as a global property by making it accessible in the preferences to set it for each and every renderer at once. This is a valid simplification as mixed rendering modes is usually something to avoid. I think the preferences service is used on multiple levels in the hierarchy described above, though, overriding rendering modes in surprising ways. What was meant as a convenience parameter in all the hierarchy above turned out to be a complexity and synchronization nightmare.

I therefore want to remove the rendering mode parameter from basically everywhere, including the currently owning mitk::BaseRenderer class, and move it to the central singleton mitk::RenderingManager to be handled as a global property as we're doing it anyway. The preference can use the public interface of this class to set/update the rendering mode for all renderers.

While investigating rendering mode issues I noticed that a rendering manager pointer is also passed to nearly every single class in the hierarchy above. I think, this parameter can be removed from all of these classes as they can access the single instance nevertheless through its static interface.

Revisions and Commits

Event Timeline

kislinsk triaged this task as Normal priority.Nov 14 2019, 4:58 PM
kislinsk created this task.
kislinsk added a project: Noteworthy.

One point I want to mention here, before I dive deeper into the problem is that we cannot use multiple rendering manager, if we rely on each class accessing the singleton rendering manager. So far this was probably never a problem but we should consider this in the future.
Having a single rendering manager means having all existing render windows available in that rendering manager and thus always updating them all e.g. on request update all.
We are not able to distinguish e.g. between the render windows of the std multi widget and another render window (e.g. a small preview render window inside a plugin view).

And you are right, the QmitkAbstracMultiWidget receives a rendering mode but each subclass (e.g. the QmitkStdMultiWidget) does not access and use it to create its QmitkRenderWindowWidget (however, here the rendering mode is passed to the QmitkRenderWindow).

I am in favor of removing the mentioned parameters (RenderingMode, RenderingManager) and find a better way to change the mode. I remember having problems with all that passing around.
However, concerning the rendering manager I wrote something in the comment above.

Thanks for the comments. The rendering manager is already designed to be a singleton and only a singleton. There's flexibilty in terms of a rendering manager factory that is used to determine the actual implementation of a rendering manager (like the QmitkRenderingManager) but still a singleton one. So I think we would not loose anything with the new approach compared to the current situation. I even think that the rendering manager should stay a singleton in the future as it is kind of a bridge to VTK and at the end of the day to the single OpenGL context of the application despite all the possible renderers (sharing that context). Calling RequestUpdateAll() all over the place is a caller issue anyway in my opinion and not a callee issue. If this should ever be a relevant performance issue with multiple editors, we could check if we really need to update specific render windows based on their visibility, if we (or even VTK) are not already doing this.

I see your point. I don't see a difference compared to the current situation neither.
I'm not specifically talking about performance issues here but rather envision a scenario where an StdMultiWidget (or any other AbstractMultiWidget) is used to display medical image for interaction purposes and another single render window, e.g. inside a DICOM preview plugin or similar, shows another medical image, e.g. and you don't want this to be reinitialized or zoomed or moved simultaneously with the interaction inside the StdMultiWidget.
This is just something to consider in the future and I also see the point in the caller vs callee issue. The new multi widget still struggles with all the RequestUpdateAll calls and similar so that it is not easy to use the new multi widget the correct way (see T26496).

Migration:

  • Remove any mitk::RenderingManager and mitk::BaseRenderer::RenderingMode::Type parameters from function calls that generate corresponding compiler errors
  • Unify access to the rendering manager by always using the static method mitk::RenderingManager::GetInstance()

TODOs:

  • Add anti-aliasing preference
kislinsk added a revision: Restricted Differential Revision.Nov 19 2019, 2:58 PM