Basti is the QA Contact for this bug.

After Global Reinit

After Reinit

I have added two pics, Thomas, is that the problem you just described? After Global reinit everything seems to be fine, after reinit the position of the 2D render windows are at its minimum extend.

Correcting 'Component', not explicitely 'Diffusion Imaging' bug.

Thomas, could you please verify if still present?

This bug is relevant for the upcoming release! Changing version and target milestone

Still present at my machine. Something is definitely wrong.

I doubt that this is a rendering bug (but I am not sure). I would assume this is a problem of geometries, since it occurs after global reinit and reinint (you have to click both!).

The brain.mhd file has a transformation matrix, which should reflect the image across the x-axis. However, if the diagonal elements of the transformation matrix are negative, the bug will occur.

Removing the sign of the first element in the brain.mhd transformation matrix results in a correct visualization. Additionally we found out, that the method mitk::RenderingManager::InitializeViews is not responsible for this bug, although the signs in the transformation matrix are investigated specifically.

(In reply to comment #9)

The brain.mhd file has a transformation matrix, which should reflect the
image across the x-axis. However, if the diagonal elements of the
transformation matrix are negative, the bug will occur.

Thus it seems that it happens for matrices that are reflections (det < 0; for pure reflection det = -1). IMO, such matrices should not be allowed for visualization, because datasets of patients with a normal anatomy would appear as situs inversus.

The reason that GlobalReinit does work (in contrast to Reinit) is probably that GlobalReinit calculates a new, axes-parallel (thus non-rotated and non-reflected) transformation using the corner-points of the geometries inside the DataStorage.

Suggestions:

• setting det<0 geometries for visualization (e.g. with SetWorldGeometry) should be rejected.
• maybe implement a method/function to create a Geometry3D with the same corner-points from an existing, "reflected" Geometry3D. Then call this method when the user selects "Reinit" in the workbench and proceed with the created Geometry3D as before.

Since this is a Geometry bug, I pass it to Basti. He can decide what to do and redirect it.

Hi there,

to my mind, it should be possible to accomodate all 48 combinations of L/R, A/P, S/I. ITK handles these orientations and has an enum for all of them. So, all the ITK code can load these images, and we do see a lot of images where the determinant is <0. The incorrect bit here is the calculation of the axial, sagittal and coronal plane location when re-init happens. The rest of the rendering will happily cope with whatever geometry. It would make the framework more confusing to debug if you implemented Ivo's second option.

We have a viewer where we drag and drop an image into the viewing window, and the axial, sagittal and coronal planes will work for all orientations, so the geometry on the image stays as it is loaded, and the 2D slice geometry is just set correctly. So, it is possible. This is a separate editor though, and I dont know how to best integrate the code in MITK.

Let me know what you want to do, and if it helps, I will see if I can dig out the code.

M

(In reply to Matt Clarkson from comment #12)

Hi there,

to my mind, it should be possible to accomodate all 48 combinations of L/R,
A/P, S/I.

I agree, of course. AFAIK, this is already possible, although probably not conveniently accessible yet (e.g., through the "View Initialization Module").

ITK handles these orientations and has an enum for all of them.
So, all the ITK code can load these images, and we do see a lot of images
where the determinant is <0. The incorrect bit here is the calculation of
the axial, sagittal and coronal plane location when re-init happens. The
rest of the rendering will happily cope with whatever geometry. It would
make the framework more confusing to debug if you implemented Ivo's second
option.

This is a very good point. May main worry was that by re-initing on one dataset with det<0 the visualization of a second dataset with det>0 will be reflected, which may be confusing. But, I think your point is more important, and there is another, better solution to my concern: Simply displaying the direction of L/R/A/P/S/I.

Thank you very much for your comment, Matt! (Unfortunately, I just read it today.)

Ivo

User hettich has pushed new remote branch:

bug-11477-lefthanded-and-righthanded-geometries

Does not work correctly, yet. We'll get it there, hopefully before christmas.

bug-11477-lefthanded-and-righthanded-geometries works now for brain.mdh and other data with lefthanded, but I'm still testing this patch. Work in progress, Ordem et Progresso, Cavet Canem, etc. pp.

(In reply to mdhettich from comment #18)

bug-11477-lefthanded-and-righthanded-geometries works now for brain.mdh and
other data with lefthanded, but I'm still testing this patch. Work in
progress, Ordem et Progresso, Cavet Canem, etc. pp.

The function ensurePerpendicularNormal() ignored lh/rh coordinate orientations
and therefore shot Reinit's assumtions to pieces. Now it seems to work, I'd like to push it so I get more and faster feedback on my patch/commit.

I plan on looking into 'Simply displaying the direction of L/R/A/P/S/I.' as well, as suggested by Matt and Ivo, that is necessary anyway IMHO.

User hettich has pushed new remote branch:

bug-11477-lefthanded-and-righthanded-geometries-integration

Thank you for figuring out the actual problem.

Displaying the directions is a subproject of Sarina's introductory project. So no redundant work here please. :)

I didn't yet have a look at the branch but pushing core changes in order to receive more feedback is not what will lead to a core modification flag. Options are, for example, to ask a few fellow bug squashers next Wednesday for testing your branch. Anyways, this seems to be a perfect case for writing tests. The latter I would require for giving the flag.

By "pushing" I meant "merging into the master". Pushing the branch is totally okay of course. Everything else would be weird. :)

(In reply to Stefan Kislinskiy from comment #22)

Thank you for figuring out the actual problem.

My pleasure.

Displaying the directions is a subproject of Sarina's introductory project.
So no redundant work here please. :)

Great!

I didn't yet have a look at the branch but pushing core changes in order to
receive more feedback is not what will lead to a core modification flag.
Options are, for example, to ask a few fellow bug squashers next Wednesday
for testing your branch. Anyways, this seems to be a perfect case for
writing tests. The latter I would require for giving the flag.

Yes, you're right of course, sorry for my overeagerness.
Tests, yes please, lets write proper unit test.
Btw, were previous tests unaware of lefthanded geometries, i wonder?

User hettich has pushed new remote branch:

bug-11477-lefthanded-and-righthanded-geometries-integration-and-unittest

Actual behavior:

see discussions above and #18114, #16772.

Expected behavior:

crosshair should not explode.
planeGeometry should consider righthanded and lefthanded coordinate
orientations / geometries and correctly calculate or handle the normal
vector.
mitkPlaneGeometry should alert user to non-perpendicular or lefthanded
geometries.

Cause of the bug:

ensurePerpendicularNormal() and InitializeStandardPlane() where ignorant
of the existence of lefthanded geometries.

Proposed solution:

Add lhOrRhSign = +1 or -1 for righthanded or lefthanded geom..
Check sign of determinant of indexToWorldTransform-matrix.
Amend ensurePerpendicularNormal() and InitializeStandardPlane()
accordingly.
Comment clearly and plentyfully.
Add MITK_DEBUG and MITK_WARN messages accordingly.

Affected classes:

mitk::PlaneGeometry.

How will the bugfix get tested:

Add Testfunction for this case to mitkPlaneGeometryTest.cpp:
TestLefthandedCoordinateSystem().

I think my patch is really ready now ^_^:
This patch fixes #11477, #16772 and #18114.

Sebastian W., Eric H., Jan. H, Esther W. and Caspar G. reviewed and helped.
C.G. built and tested on MSWindows, I on ubuntu 14.04.

Hi Martin et al., thank you for fixing this bug! Good job!
Here are some remarks:

1. Please remove the ciso646 include and stick to the regular C++ operators. We try to have a consistent coding style especially in the core.

2. Why didn't you use the cross product for detecting parallelism? The method you are using seems to complicate the code.

3. In the function EnsurePerpendiculareNormal, different functions are used to determine the length of the normal: vnl_vector.two_norm() and vnl_vector.magnitude()

Since these functions are doing the same stuff, I recommend to use one of the two versions instead of mixing both (increases readability)

4. In the function EnsurePerpendiculareNormal, you are changing the normal direction if det < 0. If the normal is not perpendicular you replace it with the adjusted one. What if the normals are parallel but the sign is different? In that case the normal would still point into the wrong direction, right?

5. In some of the InitializePlanes() functions you write, that the crossproduct is righthanded but negative spacing or slice thickness could still make it lefthanded.

I think having a negative slicethickness or spacing is very unlikely.
This is at least not very intuitive for a programmer if he wants to use these methods for creating lefthanded geometries and he has to pass a negative spacing of slicethickness. In addition, your comment regarding that is currently not part of the API documentation and hence pretty hidden. Couldn't we simply call EnsurePerpendicularNormal here?

6. Regarding the PlaneGeometryTest:
7. Wrong indentation of the braces for the tearDown() function
8. Please remove the MITK_INFOs that you added.

Just re-request the core-change-flag after you have finished the amendments.

1. Please remove the ciso646 include and stick to the regular C++ operators.

We try to have a consistent coding style especially in the core.

Done.

2. Why didn't you use the cross product for detecting parallelism? The

method you are using seems to complicate the code.

We decided it was necessary to alert the user to the case of non-perpendicular (askew) slices, of course if we alerted even in cases of numerical noise or tiny irrelevant deviations from numerically perfect parallelity that warning message would be pointless and annoying.

3. In the function EnsurePerpendiculareNormal, different functions are used

to determine the length of the normal: vnl_vector.two_norm() and
vnl_vector.magnitude()
Since these functions are doing the same stuff, I recommend to use one of
the two versions instead of mixing both (increases readability)

Done.

4. In the function EnsurePerpendiculareNormal, you are changing the normal

direction if det < 0. If the normal is not perpendicular you replace it with
the adjusted one. What if the normals are parallel but the sign is
different? In that case the normal would still point into the wrong
direction, right?

No, thats circular reasoning. The sign of the determinant and therefore
lh/rh geometry is preserved (kept) in this function, not produced: If det(transformMatrix)<0 then the normal vector zed already exists and is already lefthanded oriented. Only if the freshly calculated normal is significantly nonparallel, the last column of transformMatrix (zed) gets overwritten (with normal).

I cleaned the code layout and amended the comments to clarify.

5. In some of the InitializePlanes() functions you write, that the

crossproduct is righthanded but negative spacing or slice thickness could
still make it lefthanded.
I think having a negative slicethickness or spacing is very unlikely.
This is at least not very intuitive for a programmer if he wants to use
these methods for creating lefthanded geometries and he has to pass a
negative spacing of slicethickness. In addition, your comment regarding that
is currently not part of the API documentation and hence pretty hidden.
Couldn't we simply call EnsurePerpendicularNormal here?

There I only _commented_ to my best understanding of that code while double-checking against trouble with lh/rh coordinate orientations and my changes.
I didn't consider unlikelyness, but all possible (mathematical) cases in that existing code. Those overloaded InitializeStandardPlane() functions you mean, have no other way to use or result in lefthanded geometries, so that programmer would use an other one or directly the final one anyhow.

6. Regarding the PlaneGeometryTest:
7. Wrong indentation of the braces for the tearDown() function
8. Please remove the MITK_INFOs that you added.

done.

(In reply to mdhettich from comment #30)

4. In the function EnsurePerpendiculareNormal, you are changing the normal

direction if det < 0. If the normal is not perpendicular you replace it with
the adjusted one. What if the normals are parallel but the sign is
different? In that case the normal would still point into the wrong
direction, right?

No, thats circular reasoning. The sign of the determinant and therefore
lh/rh geometry is preserved (kept) in this function, not produced: If
det(transformMatrix)<0 then the normal vector zed already exists and is
already lefthanded oriented. Only if the freshly calculated normal is
significantly nonparallel, the last column of transformMatrix (zed) gets
overwritten (with normal).

I cleaned the code layout and amended the comments to clarify.

I see, thanks for clarification.

5. In some of the InitializePlanes() functions you write, that the

crossproduct is righthanded but negative spacing or slice thickness could
still make it lefthanded.
I think having a negative slicethickness or spacing is very unlikely.
This is at least not very intuitive for a programmer if he wants to use
these methods for creating lefthanded geometries and he has to pass a
negative spacing of slicethickness. In addition, your comment regarding that
is currently not part of the API documentation and hence pretty hidden.
Couldn't we simply call EnsurePerpendicularNormal here?

There I only _commented_ to my best understanding of that code while
double-checking against trouble with lh/rh coordinate orientations and my
changes.
I didn't consider unlikelyness, but all possible (mathematical) cases in
that existing code. Those overloaded InitializeStandardPlane() functions you
mean, have no other way to use or result in lefthanded geometries, so that
programmer would use an other one or directly the final one anyhow.

But still in these cases I suggest to add some documentation to the API (just that the overloaded versions create only righthanded orientations)

Another point just occurred to me:
The cmath header, you included is not needed. Please remove this header as well.

(In reply to Andreas Fetzer from comment #31)

But still in these cases I suggest to add some documentation to the API
(just that the overloaded versions create only righthanded orientations)

Done.

Another point just occurred to me:
The cmath header, you included is not needed. Please remove this header as
well.

Done.

Thanks alot for your help.

User hettich has pushed new remote branch:

bug-11477-Integration-HandednessAwarePlaneGeometry

(In reply to Git Admin from comment #33)

User hettich has pushed new remote branch:

bug-11477-Integration-HandednessAwarePlaneGeometry

tested on (ubuntu14.04 x86_64 g++4.7.3) und (MSWindows7 64bit MSVC2013)

[8e0b85]: Merge branch 'bug-11477-Integration-HandednessAwarePlaneGeometry'

Merged commits:

2016-01-04 14:51:30 Martin Hettich [16f825]
core change bug-11477-Integration-HandednessAwarePlaneGeometry

2015-12-22 15:01:09 Martin Hettich [6fe574]
remove unused include and amend API documentation.

2015-12-11 19:36:37 Martin Hettich [7cea1c]
cleanup and clarify to address MITK core maintainer's requests.

2015-12-02 17:13:20 Martin Hettich [62a00a]
complete comments: usages of vnl_cross_3d() in mitkPlaneGeometry.

Reviewed usages of vnl_cross_3d() (outer vector product AKA cross product),
and decided that those other two itk::CrossProduct() usages are not T11477.

2015-11-30 23:46:57 Martin Hettich [f00d2e]
correct handling of lefthanded coordinate orientations in mitkPlaneGeometry.

Amend ensurePerpendicularNormal() and InitializeStandardPlane() to check for
lefthanded coordinate systems and set the normal vectors sign accordingly,
since those methods ignored lefthanded geometries before.
Extending CppUnitTests mitkPlaneGeometryTest.cpp with TestLefthandedCoordinateSystem().
Clarifying comments and cleaning up coding style a bit.
This patch fixes #11477, #16772 and #18114.

Removed a whole bunch of former co-workers from Cc.