Fixing global RenderingManager access from QmitkRenderWindowWidget and QmitkStdMultiWidget. #248

CFornari · 2020-06-30T20:02:22Z

No description provided.

… QmitkStdMultiWidget

kislinsk · 2020-07-01T07:30:14Z

Hi, can you please explain your changes and why they are necessary? Thx!

edit: Nevermind, found your email on the mitk-users list. :-) However, there are some formatting issues and modified lines that go beyond the declared scope. Please check again to really narrow down this pull request to the absolutely necessary.

CFornari · 2020-07-01T17:12:44Z

Hi, I reviewed the code and I believe all changes are needed to solve the global access to RenderingManager. Can you indicate which line is causing trouble?

I think an explanation in the changes made on QmitkRenderWindowWidget::RequestUpdate() and QmitkRenderWindowWidget::ForceImmediateUpdate() was lacking.

The original code had those lines:

void QmitkRenderWindowWidget::InitializeGUI()
{
  ...
  mitk::TimeGeometry::ConstPointer timeGeometry = m_DataStorage->ComputeBoundingGeometry3D(m_DataStorage->GetAll());
  mitk::RenderingManager::GetInstance()->InitializeViews(timeGeometry);
  ...
}

This sets m_DataStorage's geometry to all windows in the current application, here we have two problems:

If m_DataStorage has no data loaded, the code will only mess up with all other rendering windows;
If m_DataStorage has data loaded, the code will set the geometry from current data to all windows;

Different from QmitkStdMultiWidget where I moved the geometry initialization to QmitkStdMultiWidget::ResetCrosshair(), QmitkRenderWindowWidget has no reset method, to avoid any modification in the API, I moved the geometry initialization to QmitkRenderWindowWidget::RequestUpdate() and QmitkRenderWindowWidget::ForceImmediateUpdate().

Using QmitkRenderWindowWidget looks like this:

// Creating an Axial view.
QmitkRenderWindowWidget *renderWidget1 = new QmitkRenderWindowWidget(ui->left,QString("main"),ds1);
renderWidget1->GetRenderWindow()->SetLayoutIndex(mitk::BaseRenderer::ViewDirection::AXIAL);
renderWidget1->GetSliceNavigationController()->SetDefaultViewDirection(mitk::SliceNavigationController::Axial);
renderWidget1->GetRenderWindow()->GetRenderer()->SetMapperID(mitk::BaseRenderer::Standard2D);
renderWidget1->RequestUpdate();

NOTE: I have a minimal test application for the changes in the code, but I don't know how/where I should add to the project.

CFornari · 2020-07-01T17:59:56Z

My original idea was to use QmitkRenderWindowWidget as a single axial window with mouse interaction. The first thing I noticed when using two widgets, was the global datastorage problem, which was solved with these changes.

But QmitkRenderWindowWidget is still lacking mouse interaction, decorations, methods for plane creation and a simple method for setting the view direction. I'm am actively working on a widget to solve all those problems, its already functional, I'm fixing minor bugs and I believe until next month, I will be able to make a PR and hear your comments.

akalali · 2020-07-07T07:06:21Z

So first of all thank you for your time and code suggestions. It is definitely worth taking a look at the different ways to initialize the view / geometry of the render windows.
However, I think there is some misunderstanding, looking at your last comment:

The QmitkRenderWindowWidget is an addition to and a wrapper around the QmitkRenderWindow. Its sole purpose is to add convenience-functions for the render window to allow to easily modify associated properties like decoration colors, background colors, corner annotations and crosshair.
The QmitkRenderWindowWidget is not supposed to take care of mouse interaction, plane create of view direction handling. All this can be done directly on the render window or is done inside one of the multi widgets (see QmitkAbstractMultiWidget, QmitkStdMultiWidget or QmitkMxNMultiWidget).

I will review your changes but in the meantime it might be useful to understand your use case and why you think it is necessary to extend the render window widget.

akalali · 2020-07-07T07:08:48Z

Modules/QtWidgets/src/QmitkRenderWindowWidget.cpp

-    mitk::BaseRenderer::GetInstance(m_RenderWindow->GetRenderWindow())->SetDataStorage(dataStorage);
-  }
+		mitk::BaseRenderer::GetInstance(m_RenderWindow->GetRenderWindow())->SetDataStorage(dataStorage);
+	}


Nothing changed here. Please don't change formatting.

Sorry for the indentation, in my IDE everything looks normal.This happened after my push to github, I will see how to solve this formatting issue.

akalali · 2020-07-07T07:09:03Z

Modules/QtWidgets/src/QmitkStdMultiWidget.cpp

@@ -633,7 +637,7 @@ void QmitkStdMultiWidget::AddDisplayPlaneSubTree()
  // ... of widget 1
  mitk::BaseRenderer* renderer1 = mitk::BaseRenderer::GetInstance(GetRenderWindow1()->GetRenderWindow());
  m_PlaneNode1 = renderer1->GetCurrentWorldPlaneGeometryNode();
-  m_PlaneNode1->SetProperty("visible", mitk::BoolProperty::New(true));
+	m_PlaneNode1->SetProperty("visible", mitk::BoolProperty::New(true));


Nothing changed here. Please don't change formatting.

akalali · 2020-07-07T07:45:58Z

Modules/QtWidgets/src/QmitkRenderWindowWidget.cpp

@@ -196,8 +198,8 @@ void QmitkRenderWindowWidget::InitializeGUI()

  connect(m_RenderWindow, &QVTKOpenGLWidget::mouseEvent, this, &QmitkRenderWindowWidget::MouseEvent);

-  mitk::TimeGeometry::ConstPointer timeGeometry = m_DataStorage->ComputeBoundingGeometry3D(m_DataStorage->GetAll());
-  mitk::RenderingManager::GetInstance()->InitializeViews(timeGeometry);
+	mitk::BaseRenderer::GetInstance(m_RenderWindow->GetRenderWindow())->SetDataStorage(m_DataStorage);


I agree setting the data storage individually for the current render window is the correct way to do it (as it is done in QmitkRenderWindowWidget::SetDataStorage).

akalali · 2020-07-07T07:54:20Z

Modules/QtWidgets/src/QmitkStdMultiWidget.cpp

+	mitk::RenderingManager::GetInstance()->InitializeView(GetRenderWindow1()->GetRenderWindow(), geo);
+	mitk::RenderingManager::GetInstance()->InitializeView(GetRenderWindow2()->GetRenderWindow(), geo);
+	mitk::RenderingManager::GetInstance()->InitializeView(GetRenderWindow3()->GetRenderWindow(), geo);
+	mitk::RenderingManager::GetInstance()->InitializeView(GetRenderWindow4()->GetRenderWindow(), geo);


What is your idea here? You are still initializing each render window to the same geometry, the bounding geometry of every data object in the data storage. This is what mitk::RenderingManager::GetInstance()->InitializeViewsByBoundingObjects(dataStorage); is actually supposed to do.

If we are displaying QmitkStdMultiWidget and QmitkRenderWindow both at the same time, each one rendering a different data set, mitk::RenderingManager::GetInstance()->InitializeViewsByBoundingObjects(dataStorage); would set the geometry from datastorage rendered by QmitkStdMultiWidget to QmitkRenderWindow, the last one may contain different data with different geometries. This is not a issue in MitkWorkbench because, usually, QmitkStdMultiWidget is the only active render widget.

So you are explicitly initializing only the four render windows of the QmitkStdMultiWidget so that other render windows (that are registered with the global rendering manager) are not affected here.
I think that is reasonable and makes sense here.

akalali · 2020-07-07T07:55:38Z

Modules/QtWidgets/src/QmitkStdMultiWidget.cpp

@@ -742,7 +744,7 @@ void QmitkStdMultiWidget::CreateRenderWindowWidgets()
  auto renderWindow2 = renderWindowWidget2->GetRenderWindow();
  renderWindow2->GetSliceNavigationController()->SetDefaultViewDirection(mitk::SliceNavigationController::Sagittal);
  renderWindowWidget2->SetDecorationColor(GetDecorationColor(1));
-  renderWindowWidget2->setStyleSheet("border: 0px");
+//  renderWindowWidget2->setStyleSheet("border: 0px");


Please remove instead of commenting out.

akalali · 2020-07-07T08:42:05Z

Modules/QtWidgets/src/QmitkRenderWindowWidget.cpp

@@ -196,8 +198,8 @@ void QmitkRenderWindowWidget::InitializeGUI()

  connect(m_RenderWindow, &QVTKOpenGLWidget::mouseEvent, this, &QmitkRenderWindowWidget::MouseEvent);

-  mitk::TimeGeometry::ConstPointer timeGeometry = m_DataStorage->ComputeBoundingGeometry3D(m_DataStorage->GetAll());
-  mitk::RenderingManager::GetInstance()->InitializeViews(timeGeometry);


Why did you remove this instead of using the code that you added in RequestUpdate and ForceImmediateUpdate:

auto geo = m_DataStorage->ComputeBoundingGeometry3D(m_DataStorage->GetAll()); mitk::RenderingManager::GetInstance()->InitializeView(m_RenderWindow->GetRenderWindow(), geo);

This will initialize the view for this specific render window.

Here we have two problems:

mitk::RenderingManager::GetInstance()->InitializeViews(timeGeometry); has global scope, it creates the same problem that we have with:
mitk::RenderingManager::GetInstance()->InitializeViewsByBoundingObjects(dataStorage);
See the example I described below. To avoid this I choose to use InitializeView() instead of InitializeViews().

I moved the location to RequestUpdate and ForceImmediateUpdate, because in the case where data is loaded dynamically, lets say the user clicks an open file button, in this case m_DataStorage wouldn't have any data at QmitkRenderWidget's construction time, in which case computing the geometry from an empty m_DataStorage wouldn't be necessary. Maintaining this piece of code here is actually useful if the path to the data set is specified through command line or has been hardcoded.

I'm totally fine with 1.
I think 2. is a fix after changing the render window initialization of the QmitkStdMultiWidget: Here we do not re-initialize all render windows anymore but only the four render windows of the multi widget. So a single render window widget won't be affected. We need to be able to explicitly re-initialize a single render window widget, right?

Would it make sense to request an update inside InitializeGUI in case we actually do have some data loaded in the data storage?

Would it make sense to request an update inside InitializeGUI in case we actually do have some data loaded in the data storage?

Yes it will make sense, I will add a geometry initialization inside the function.

I think 2. is a fix after changing the render window initialization of the QmitkStdMultiWidget:

Actually I explicitly initialized the geometries inside QmitkStdMultiWidget, would you prefer a call to RequestUpdate instead of mitk::RenderingManager::GetInstance()->InitializeView(GetRenderWindow1()->GetRenderWindow(), geo);? The result will be the same.

Here we do not re-initialize all render windows anymore but only the four render windows of the multi widget. So a single render window widget won't be affected. We need to be able to explicitly re-initialize a single render window widget, right?

Yes this is the intention, although I ended up not calling RequesUpdate.

akalali · 2020-07-07T08:42:32Z

Modules/QtWidgets/src/QmitkRenderWindowWidget.cpp

@@ -63,11 +63,15 @@ mitk::SliceNavigationController* QmitkRenderWindowWidget::GetSliceNavigationCont

 void QmitkRenderWindowWidget::RequestUpdate()
 {
+	auto geo = m_DataStorage->ComputeBoundingGeometry3D(m_DataStorage->GetAll());
+	mitk::RenderingManager::GetInstance()->InitializeView(m_RenderWindow->GetRenderWindow(), geo);


Why not put this inside the InitializeGUI-function?

akalali · 2020-07-07T08:42:48Z

Modules/QtWidgets/src/QmitkRenderWindowWidget.cpp

  mitk::RenderingManager::GetInstance()->RequestUpdate(m_RenderWindow->GetRenderWindow());
 }

 void QmitkRenderWindowWidget::ForceImmediateUpdate()
 {
+	auto geo = m_DataStorage->ComputeBoundingGeometry3D(m_DataStorage->GetAll());
+	mitk::RenderingManager::GetInstance()->InitializeView(m_RenderWindow->GetRenderWindow(), geo);


Why not put this inside the InitializeGUI-function?

akalali

Thank you again.

We have an experimental version of a QmitkMxNMultiWidget that allows to have a more general set of render window widgets. After we included your PR it might be worth checking this widget again to see how it behaves with the changes concerning the view initialization of specific render window widgets.

CFornari · 2020-07-22T02:41:11Z

Thanks for your patience.
I didn't knew about QmitkMxNMultiWidget I will take a look at it.

akalali · 2022-09-07T14:41:51Z

@CFornari
This has been on hold for looong time now but we are currently working on the mentioned QmitkMxNMultiWidget. While working on that / refactoring existing code we realized that some of the changes done here might be useful. I would like to include this pull request in our recent development.
However, since some times has passed, changes might be required.

Carlo added 3 commits June 30, 2020 16:50

Fixed RenderingManager global access from QmitkRenderWindowWidget and…

8891177

… QmitkStdMultiWidget

Deleted unecessary comments.

f09195e

Deleted more unecessary comments.

b5f07fd

akalali suggested changes Jul 7, 2020

View reviewed changes

akalali approved these changes Jul 15, 2020

View reviewed changes

Changes as discussed in the pull request 248.

79883a5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing global RenderingManager access from QmitkRenderWindowWidget and QmitkStdMultiWidget. #248

Fixing global RenderingManager access from QmitkRenderWindowWidget and QmitkStdMultiWidget. #248

CFornari commented Jun 30, 2020

kislinsk commented Jul 1, 2020 •

edited

CFornari commented Jul 1, 2020

CFornari commented Jul 1, 2020

akalali commented Jul 7, 2020

akalali Jul 7, 2020

CFornari Jul 7, 2020

akalali Jul 7, 2020

akalali Jul 7, 2020

akalali Jul 7, 2020

CFornari Jul 7, 2020

akalali Jul 15, 2020

akalali Jul 7, 2020

akalali Jul 7, 2020

CFornari Jul 7, 2020 •

edited

akalali Jul 15, 2020

CFornari Jul 22, 2020

akalali Jul 7, 2020

akalali Jul 7, 2020

akalali left a comment

CFornari commented Jul 22, 2020

akalali commented Sep 7, 2022 •

edited

Fixing global RenderingManager access from QmitkRenderWindowWidget and QmitkStdMultiWidget. #248

Are you sure you want to change the base?

Fixing global RenderingManager access from QmitkRenderWindowWidget and QmitkStdMultiWidget. #248

Conversation

CFornari commented Jun 30, 2020

kislinsk commented Jul 1, 2020 • edited

CFornari commented Jul 1, 2020

CFornari commented Jul 1, 2020

akalali commented Jul 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CFornari Jul 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akalali left a comment

Choose a reason for hiding this comment

CFornari commented Jul 22, 2020

akalali commented Sep 7, 2022 • edited

kislinsk commented Jul 1, 2020 •

edited

CFornari Jul 7, 2020 •

edited

akalali commented Sep 7, 2022 •

edited