Page MenuHomePhabricator

SlicesRotator not working with multiple RenderWindows (was designed only with "MultiWidget" in mind)
Closed, ResolvedPublic

Assigned To
None
Authored By
maleike
Oct 3 2012, 12:41 AM
Referenced Files
F914: all-changes-in-one-patch.diff
Oct 3 2012, 12:52 AM
F913: patch-step8.diff
Oct 3 2012, 12:44 AM
Subscribers

Description

The mitk::SlicesRotator works fine with the QmitkStdMultiWidget setup, which is three 2D views and one 3D view. As soon as somebody uses the class in order to coordinate four 2D views (axial, sagittal, frontal, axial clone of the first view) it will fail. Nothing will be done in reaction to mouse input because there are some hard-coded expectations about exactly two orthogonal planes being visible. These expectations will fail whenever there is a fourth render window.

To fix this, I basically reviewed and refactored the whole class:

  • the long switch/case block in ExecuteAction was distributed into methods
  • some 20 lines which would be used to determine a value that is used in an if-clause and would always(!) evaluate to true was removed
  • the whole "check whether we want to rotate slices or move the crosshair" logic was re-thought, documented first, then implemented as intended

The modified class works fine in a setting like described above.

To make review of this bug easier, I'll attach a patch to tutorial step 8 which will modify the tutorial application in a way that it has two axial views.

Since this is a rather big change to an important class, I'd like if a second person would review and test the code before we integrate it. Basti, since you worked much with geometries, could you perhaps look into this? Otherwise this would be a nice bugsquashing-review.

Event Timeline

maleike added a subscriber: maleike.

I pushed a branch with the proposed fix/refactoring into bug-13329-refactor-slices-rotator

Ok, I'm behind a firewall, so I'll do that as soon as possible

Patch to Step8.cpp to make it display two axial views

There is one open issue, which I did not / could not solve yet. If you have some idea, please document during review. I would propose, however, to postpone this issue and fix in a separate bug:

IF there are multiple crossings of other renderwindow planes in a render window, we will get multiple possible centers of rotation! In this case the implemented behavior is undefined. The code will pick any of those centers. I have no idea what an appropriate solution could look like. (Perhas one would pick the closest possible center of rotation??)

Anyway, I'll push that branch and I'm curious for comments.

Until I can push the branch, here is a patch that contains all changes

Branch is now long pushed as bug-13329-refactor-slices-rotator. I just updated the branch to reflect recent minor changes from T10180. Branch merges again into master without conflicts, all unit tests pass. I'm waiting one more day for comments of Bastian (comment requeste by mail now).

[7a365a]: Merge branch 'bug-13329-refactor-slices-rotator'

Merged commits:

2012-10-18 15:50:44 Daniel Maleike [241b27]
Adapting changes of T10180 to avoid conflicts and not throw away Stefan's work


2012-10-18 15:48:11 Daniel Maleike [a115a0]
Merge remote-tracking branch 'origin/bug-10180-MemoryLeakWhenUpdating2DImages' into bug-13329-refactor-slices-rotator


2012-10-03 00:41:44 Daniel Maleike [01d626]
Fix brief class documentation


2012-10-03 00:29:11 Daniel Maleike [eccb06]
Re-write / Refactoring of the SlicesRotator; tried to document much more of it; generalize to more than 3 planes


2012-10-02 20:59:08 Daniel Maleike [c96e1e]
Document and clean up SlicesRotator, remove obsolete code; unchanged behavior

The fix is integrated now. Tests on continuous dartclients are fine. If Bastian comes up with findings during review, we can reopen this ticket.