User wirkert has pushed new remote branch:

bug-18034-proposal_for_removal_of_redundant_data

A quick search in my archives didn't reveal a dedicated discussion about this topic yet.

There is this page which talks about BaseGeometry concepts but I am not sure if this is the current state or future design:

http://www.mitk.org/internal/Refactoring%20of%20Geometry-Concept/NewDesign

The page of the new design was created before my platform project. The first part (time geometry) is - as far as I know - imlemented like it is stated on this page. The second part (BaseGeometry) was only a suggestion for my platform project and was implemented slightly different.

In my opinion, sepatarion of the variables should be possible and go even further. Also consider removement of m_vtkmatrix and m_vtkindextoworldtransform. Of course, Get- and Set-methods should still be avaiable, but these values can be calculated if needed using the AffineTransform.

Also, all transformations (WorldToIndex, IndexToWorld) currently use the AffineTransform and NOT the variables m_origin or m_spacing. This shouldn't cause any problems. Only the method ExecuteOperation needs careful adaption (here, the vtkMatrix is used).

Outside of the geometry classes, these removements shouldn’t change the behavior, because all these variables are private since the introduction of the new geometry concept.

As Esther might know i really like the idea of cleaning the saving of the geometries. This is a really important issue and has already been discussed sometimes. I think the new geometry structure will make it possible.

Just some thoughts on this issue:

• The rendering makes rather extensive use of Vtk-Methods. So it might be a performance issue not to store them. But they might be calculated if a new AffineTransform is stored and never else.

• m_Origins and m_Spacing are pretty good for debugging. Reading a Matrix to get these values is not always straight forward.

• There should be a clear way to change the origin, spacing etc... without changing the rest of the transformation matrix. But this should be possible.

User wildes has pushed new remote branch:

bug-18034-proposal_for_removal_of_redundant_data-IntegrBranch

User wildes has pushed new remote branch:

bug-18034-proposal_for_removal_of_redundant_data-IntegrBranch2

Tests pass under Windows and Linux.

Pre-liminary code review:

1. Do we need to expose the new GeometryTransformHolder class in the public API of BaseGeometry? Could it me made an implementation detail and not exported (no use of MITK_CORE_EXPORT)?

2. There seems to be no value in overwriting the pre/post methods of BaseGeometry using empty or "superclass" implementations (maybe I overlooked it). Can these methods be removed from
   • AbstractTransformGeometry
   • Geometry3D
   • DisplayGeometry
   • LandmarkProjectorBasedCurvedGeometry
   • PlaneGeometry
   • SlicedGeometry3D
   • ThinPlateSplineCurvedGeometry

?

Answer to 1.:
Probably you're right. We'll check it on Wednesday.

Answer to 2.:
We do need to implement the pre-/post functions in every method. This was a bug discovered by Michael Brehler (maybe Michael can explain more about it?).

The reason is the following:
If you inherit twice from a class, and the one in the "middle" has no implementation of this pre/post function, the lowest class is not able to call the code of the original superclass. This leads to wrong initialization in mitkThinPlateSplineCurvedGeometry, which inherits from the following classes:
ThinPlateSplineCurvedGeometry -> LandmarkProjectedBasedCurvedGeometry -> AbstractTransformGeometry -> PlaneGeometry -> BaseGeometry.

Code implemented in the AbstractTransformGeometry (PostInitialize) was not called when creating a new ThinPSCGeo. The only way we found to solve this behavior, was to implement an empty function in every subclass.

The reason why we want to call superclass in every subclass is, that we want to preserve a consistent behavior of subclasses.

The only other solution I see is to get rid of the pre/post functions. But I'm not sure if this is possible at all and needs a lot of restructuring. Some of it is part of T18520.

Answer to 1.: Core export is removed, class is indirect tested via BaseGeometry tests.

2.) Got it. It is indeed related to the famous "brittle base class" problem... I don't see a better solution except for refactoring the complete pre/post method approach (as you mentioned). Could we have a convenience macro which implements the standard pre/post methods so that future refactoring work is not spread over all BaseGeometry sub-classes but instead only touches that macro (and the base class)?

User wildes has pushed new remote branch:

bug-18034-proposal_for_removal_of_redundant_data-IntegrBranch3

[0581db]: Merge branch 'bug-18034-proposal_for_removal_of_redundant_data-IntegrB

Merged commits:

2015-01-07 13:26:47 Esther Wild [a78688]
Merge branch 'bug-18034-proposal_for_removal_of_redundant_data-IntegrBranch2' into bug-18034-proposal_for_removal_of_redundant_data-IntegrBranch3

2014-12-10 17:00:13 Esther Wild [e1b8f5]
removed core export of GeometryTransformHolder class.
Test is also deleted, testing of TransformHolder is indirect included in BaseGeometryTest.

2014-12-03 16:22:09 Esther Wild [e49f02]
Wrong if condition... happens...

2014-12-03 16:07:00 Esther Wild [fc8da6]
Remove comment

2014-12-03 15:36:49 Esther Wild [759ae4]
Added new feature: Set ITWTransfom without changing the spacing. Tests included.

2014-11-26 15:27:04 Esther Wild [c98fd1]
Merge branch 'bug-18034-proposal_for_removal_of_redundant_data-IntegrBranch' into bug-18034-proposal_for_removal_of_redundant_data-IntegrBranch2

Conflicts:
Core/Code/Testing/mitkBaseGeometryTest.cpp
Modules/IpPicSupport/mitkPicHelper.cpp

2014-10-01 15:04:14 Sebastian Wirkert [4cfaa7]
Adapted some documentation and added a few checks in GeometryTransformHolderTest.

2014-09-24 16:08:54 Esther Wild [530398]
Test for GeometryTransformHolder is working now.

Small bugfix in TransformHolder class (no proper initialization).

2014-09-18 18:04:30 Esther Wild [25d030]
New GeometryTransformHolderTest

!Not compiling!

2014-09-18 18:01:34 Esther Wild [dd5c11]
New Handling of Pre-/Post-functions

!Not compiling yet!

2014-09-18 17:58:50 Esther Wild [4ccbb7]
Merge branch 'bug-18034-proposal_for_removal_of_redundant_data' into bug-18034-proposal_for_removal_of_redundant_data-IntegrBranch

Conflicts:
Core/Code/Testing/mitkBaseGeometryTest.cpp

2014-09-10 16:43:06 Esther Wild [1e21bd]
Cleaned BackTransform function.

2014-09-10 14:58:11 Sebastian Wirkert [2c4770]
nicer handling of + and * without loops.

2014-09-10 14:51:00 Sebastian Wirkert [f642cb]
cleaned up code

1. deleted unnecessary commentaries
2. some other small changes.

2014-09-10 14:16:36 Sebastian Wirkert [f8481b]
Cleaned up initilizing. Now All initialization of GeometryTransformHolder is
actually done in GeometryTransformHolder.

2014-09-03 16:31:09 Sebastian Wirkert [43bc46]
New file for GeometryTransform (now known as GeometryTransformHolder),
because it was hard to read while this file was still in
BaseGeometry.cpp.
Fixed some errors, now all tests run through again.

2014-08-29 18:16:32 Esther Wild [ec820a]
Three-quarter finished restructuring of BaseGeometry: Compiling but not running yet...

2014-08-27 17:03:01 Sebastian Wirkert [2252ae]
Half finished restructuring of basegeometry:
capsuling all transfrom relevent data into inner class GeometryTransform.
Adapted functions Get/Set Origin/Spacing.
Moved TransferItk/Vtk functions and CopySpacing.

2014-08-27 13:53:58 Sebastian Wirkert [a76922]
Reintroduced parameter for SetSpacing (enforceSetSpacing), now all tests pass.

2014-08-20 17:52:00 Sebastian Wirkert [07b327]
Removed Origin and Spacing, 3 tests fail, investigate!