Re: [mitk-users] QmitkRenderWindowWidget design flaw: Setting datastorage globally.

2020-06-30 Thread Kalali, Amir
Hello Carlo,

Thank you for pointing that out. I took a look at it and I think you’re right, 
there seems to be a mixture of the individual rendering manager (which might or 
might not be the same for every render window widget) and the global, singleton 
rendering manager.

The QmitkRenderWindowWidget was introduced while refactoring the rendering 
widgets to allow for a more flexible multi widget editor. Such a flexible multi 
widget editor should be able to display different image in parallel (with 
different geometry / view direction etc.) and thus it makes sense to also allow 
different data storages and rendering managers.

I’m happy to see your changes contributed, just fork 
https://github.com/MITK/MITK and create a pull request. I looked at your 
changes and I would like to comment on it, which is easy having a PR.

Best,

Amir

From: Carlo [mailto:cv.forn...@gmail.com]
Sent: Thursday, 4 June, 2020 23:16
To: mitk-users@lists.sourceforge.net
Subject: [mitk-users] QmitkRenderWindowWidget design flaw: Setting datastorage 
globally.

Hello,
It's been a month since I started to develop an application using MITK as a 
toolkit. I need to render two independent 3D windows, to achieve this I choose 
the QmitkRenderWindowWidget, but inside QmitkRenderWindowWidget.cpp we have the 
method:
(1) void QmitkRenderWindowWidget::InitializeGUI();

This is a private method called by the constructor and inside it we have this 
line of code:



(2) mitk::RenderingManager::GetInstance()->SetDataStorage(m_DataStorage);



This code sets m_DataStorage's content globally, and at the moment we declare 
QmitkRenderWindowWidget, all other rendering windows are modified.



The behavior seems to be a design flaw, because QmitkRenderWindowWidget has a 
method:



(3) void QmitkRenderWindowWidget::SetDataStorage(mitk::DataStorage* 
dataStorage);



I can't tell for sure, but this method seems to set the datastorage only for 
the current QmitkRenderWindowWidget. So there is no need for (2).



Here is the next thing that took my attention:



QmitkStdMultiWidget inherits QmitkAbstractMultiWidget, the last one has a 
method:



(4) void QmitkStdMultiWidget::SetDataStorage(mitk::DataStorage* dataStorage);



QmitkStdMultiWidget has four rendering widgets, when calling method (4), there 
are four callings for method (3), one for each rendering widget,

but their constructors had already called four times the method (2), and this 
last method has messed up every other rendering window in the current 
application.



For these reasons, I believe method (2) should be removed as it is there 
probably by mistake. Also, the algorithms for widgets initialization doesn't

appear to consider method (2) existence.



I'd welcome any thoughts on this.
___
mitk-users mailing list
mitk-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mitk-users


Re: [mitk-users] QmitkRenderWindowWidget design flaw: Setting datastorage globally.

2020-06-30 Thread Carlo
Hello Amir,

As you have suggested, here is the PR: https://github.com/MITK/MITK/pull/248
.

I've made some changes in those files I sent you previously, also removed
some misleading comments I placed during the tests I did.

I created some minimal test applications, if you think this should be added
I'd appreciate guidance on how to add it to the project.

Cheers,

Carlo.


On Tue, Jun 30, 2020 at 5:39 AM Kalali, Amir 
wrote:

> Hello Carlo,
>
>
>
> Thank you for pointing that out. I took a look at it and I think you’re
> right, there seems to be a mixture of the individual rendering manager
> (which might or might not be the same for every render window widget) and
> the global, singleton rendering manager.
>
>
>
> The QmitkRenderWindowWidget was introduced while refactoring the rendering
> widgets to allow for a more flexible multi widget editor. Such a flexible
> multi widget editor should be able to display different image in parallel
> (with different geometry / view direction etc.) and thus it makes sense to
> also allow different data storages and rendering managers.
>
>
>
> I’m happy to see your changes contributed, just fork
> https://github.com/MITK/MITK and create a pull request. I looked at your
> changes and I would like to comment on it, which is easy having a PR.
>
>
>
> Best,
>
>
>
> Amir
>
>
>
> *From:* Carlo [mailto:cv.forn...@gmail.com]
> *Sent:* Thursday, 4 June, 2020 23:16
> *To:* mitk-users@lists.sourceforge.net
> *Subject:* [mitk-users] QmitkRenderWindowWidget design flaw: Setting
> datastorage globally.
>
>
>
> Hello,
>
> It's been a month since I started to develop an application using MITK as
> a toolkit. I need to render two independent 3D windows, to achieve this I
> choose the QmitkRenderWindowWidget, but inside QmitkRenderWindowWidget.cpp
> we have the method:
>
> (1) void QmitkRenderWindowWidget::InitializeGUI();
>
>
>
> This is a private method called by the constructor and inside it we have
> this line of code:
>
>
>
> (2) mitk::RenderingManager::GetInstance()->SetDataStorage(m_DataStorage);
>
>
>
> This code sets m_DataStorage's content globally, and at the moment we declare 
> QmitkRenderWindowWidget, all other rendering windows are modified.
>
>
>
> The behavior seems to be a design flaw, because QmitkRenderWindowWidget has a 
> method:
>
>
>
> (3) void QmitkRenderWindowWidget::SetDataStorage(mitk::DataStorage* 
> dataStorage);
>
>
>
> I can't tell for sure, but this method seems to set the datastorage only for 
> the current QmitkRenderWindowWidget. So there is no need for (2).
>
>
>
> Here is the next thing that took my attention:
>
>
>
> QmitkStdMultiWidget inherits QmitkAbstractMultiWidget, the last one has a 
> method:
>
>
>
> (4) void QmitkStdMultiWidget::SetDataStorage(mitk::DataStorage* dataStorage);
>
>
>
> QmitkStdMultiWidget has four rendering widgets, when calling method (4), 
> there are four callings for method (3), one for each rendering widget,
>
> but their constructors had already called four times the method (2), and this 
> last method has messed up every other rendering window in the current 
> application.
>
>
>
> For these reasons, I believe method (2) should be removed as it is there 
> probably by mistake. Also, the algorithms for widgets initialization doesn't
>
> appear to consider method (2) existence.
>
>
>
> I'd welcome any thoughts on this.
>
>
___
mitk-users mailing list
mitk-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mitk-users