Page MenuHomePhabricator
Create Task
Maniphest T28617

Inconsistent view initialization in rendering manager
Closed, ResolvedPublic
Actions

Description

Looking at the mitk::RenderingManager there are several functions that are basically doing the same. They only differ in their given parameters and therefore need to process the given data slightly differently.
However, there is sometimes a different logic although they should do the same and I wonder why that is, e.g.:

https://phabricator.mitk.org/source/mitk/browse/develop/Modules/Core/src/Controllers/mitkRenderingManager.cpp$447 uses initializeGlobalTimeSNC whereas https://phabricator.mitk.org/source/mitk/browse/develop/Modules/Core/src/Controllers/mitkRenderingManager.cpp$374 does not use this variable.

On the other hand,
https://phabricator.mitk.org/source/mitk/browse/develop/Modules/Core/src/Controllers/mitkRenderingManager.cpp$331 extends the bounding box to have "an extent bigger than zero in any direction" whereas
the function in https://phabricator.mitk.org/source/mitk/browse/develop/Modules/Core/src/Controllers/mitkRenderingManager.cpp$426 does no such thing.

Also, only in https://phabricator.mitk.org/source/mitk/browse/develop/Modules/Core/src/Controllers/mitkRenderingManager.cpp$385 mitk::RenderingManagerViewsInitializedEvent() is called, although a a rendering request / view initialization happened in other functions as well.

There is also an old comment in https://phabricator.mitk.org/source/mitk/browse/develop/Modules/Core/src/Controllers/mitkRenderingManager.cpp$297 that says "Remove old function, so only this one is working." so maybe that's a hint to remove the later function?

I found this while working on D517 to separate the rendering from the view initialization.

If we can clarify this we might be able to remove many lines of code and simplify the code that is concerned with view initialization, which would bring us a step forward in finding a good solution for related tasks, such as T28490, T27613 and T26496

Revisions and Commits

Restricted Differential Revision
rMITK MITK
Restricted Differential RevisionrMITK0905c8748485 Always initialize global time
Restricted Differential RevisionrMITK476ec6d312f1 Remove initializeGlobalTime parameter

Related Objects

Event Timeline

kalali triaged this task as High priority.Jul 9 2021, 4:14 PM
kalali created this task.
kalali added a comment.EditedJul 9 2021, 5:22 PM

The latter function bool RenderingManager::InitializeView(vtkRenderWindow *renderWindow, const TimeGeometry *geometry, bool initializeGlobalTimeSNC), using the initializeGlobalTimeSNC is rarely used at all, only in

  • QmitkToFUtilView::UseToFVisibilitySettings
  • QmitkDataNodeOpenInAction::OnActionTriggered
  • ReinitAction::Run
  • LabelSetJumpToAction::Run

I tested this function using initializeGlobalTimeSNC = false, so that m_TimeNavigationController->SetInputWorldTimeGeometry(geometry); is not called. Using a 4D image the time slicer is not correctly initialized and cannot be used / changed. I wonder in which scenario one would disable setting the correct input world time geometry!? Obviously this function was not correctly used (also by myself) and the default value for this parameter was simply accepted, although it is set to false.

Simply removing this function does not work since it's the only function that allows to pass a specific single render window. And it will be used more often when using the mxn-multi widget.
However, it is not complicated to refactor these functions to remove this parameter and to simplify the functions to call each-other to allow passing a single render window.

kalali moved this task from Backlog to Cycle on the MITK (v2021.10) board.Jul 12 2021, 9:38 AM
kislinsk edited projects, added MITK (v2022.04); removed MITK (v2021.10).Nov 10 2021, 4:32 PM
kalali moved this task from Backlog to Cycle on the MITK (v2022.04) board.Jan 18 2022, 5:44 PM
kalali added a project: Request for Discussion.
kalali added a comment.Jan 19 2022, 12:04 PM
In T28617#226314, @kalali wrote:

Simply removing this function does not work since it's the only function that allows to pass a specific single render window. And it will be used more often when using the mxn-multi widget.
However, it is not complicated to refactor these functions to remove this parameter and to simplify the functions to call each-other to allow passing a single render window.

@floca This task is also relevant in the context of D517. We can discuss this and refactor the mentioned functions / parameters within the open differential.

kalali claimed this task.Feb 9 2022, 11:42 AM
kalali mentioned this in rMITK5a894d07de8d: Improve reinit behavior.Mar 8 2022, 2:39 PM
kalali updated the task description. (Show Details)Mar 8 2022, 3:05 PM
kalali removed a project: Request for Discussion.
kalali added a revision: Restricted Differential Revision.Mar 8 2022, 3:23 PM
kalali added a revision: Restricted Differential Revision.Mar 9 2022, 12:48 PM
In T28617#226314, @kalali wrote:

The latter function bool RenderingManager::InitializeView(vtkRenderWindow *renderWindow, const TimeGeometry *geometry, bool initializeGlobalTimeSNC), using the initializeGlobalTimeSNC is rarely used at all, only in

  • QmitkToFUtilView::UseToFVisibilitySettings
  • QmitkDataNodeOpenInAction::OnActionTriggered
  • ReinitAction::Run
  • LabelSetJumpToAction::Run

I tested this function using initializeGlobalTimeSNC = false, so that m_TimeNavigationController->SetInputWorldTimeGeometry(geometry); is not called. Using a 4D image the time slicer is not correctly initialized and cannot be used / changed. I wonder in which scenario one would disable setting the correct input world time geometry!? Obviously this function was not correctly used (also by myself) and the default value for this parameter was simply accepted, although it is set to false.

Simply removing this function does not work since it's the only function that allows to pass a specific single render window. And it will be used more often when using the mxn-multi widget.
However, it is not complicated to refactor these functions to remove this parameter and to simplify the functions to call each-other to allow passing a single render window.

This one is remaining and I still can't make any sense of this additional parameter.
I added another differential that removes this parameter: D608
Btw., I found one more occurrence of this function call (using the default parameters):

  • mitk::RenderWindowViewDirectionController::InitializeViewByBoundingObjects

Since all mentioned calls are using the default parameter true (and were using the argument true before, when the default parameter was still false), the calls do not have to be modified.

kalali added a project: Breaking Change.Mar 20 2022, 9:16 PM
IMPORTANT: virtual bool RenderingManager::InitializeView(vtkRenderWindow *renderWindow, const BaseGeometry *geometry, bool initializeGlobalTime = true, bool resetCamera = true); was changed to virtual bool RenderingManager::InitializeView(vtkRenderWindow *renderWindow, const BaseGeometry *geometry, bool resetCamera = true);.
IMPORTANT: virtual bool RenderingManager::InitializeView(vtkRenderWindow *renderWindow, const TimeGeometry *geometry, bool initializeGlobalTime = true, bool resetCamera = true); was changed to virtual bool RenderingManager::InitializeView(vtkRenderWindow *renderWindow, const TimeGeometry *geometry, bool resetCamera = true);.
kalali added a commit: rMITK476ec6d312f1: Remove initializeGlobalTime parameter.Mar 20 2022, 9:18 PM
kalali added a commit: rMITK0905c8748485: Always initialize global time.
kalali closed this task as Resolved.Mar 20 2022, 9:18 PM
kislinsk mentioned this in T29046: 2022 Week 12 (Late March).Mar 23 2022, 11:23 AM
kalali mentioned this in T27613: Improve reinit behavior.May 19 2022, 6:48 PM
MITK · MIC · IMSY · DKFZ