Page MenuHomePhabricator

Geometry of PointSet inconsistent and not considered in PointSetWriter
Closed, ResolvedPublic

Description

Usecase: I am doing quite the same as in PointBaseRegistration plugin. Two pointsets are registered which leads to a vtkMatrix4x4 that is to be set to a third data node selected afterwards. As in the mentioned plugin I do:

...
mitk::Geometry3D::Pointer movingGeometry = movingPointSet->GetGeometry(0);
movingGeometry->Compose(m_RegistrationMatrix);
movingPointSet->GetTimeSlicedGeometry()->UpdateInformation();
mitk::RenderingManager::GetInstance()->RequestUpdateAll();

Bug: I: In 3D widget the moving pointset is shown as expected but not in 2D views (!?!). So visualization is broken?
II: Then when saving the moved pointset the geometry is not considered. The saved transformed mps file contains the same coordinates as the non transformed one.

You can reproduce this by using the mitk PointBasedRegistration Plugin. MovingPointSet is moved in 3D views but not in 2D views (see attached image).

I used a patched version of 2013.06.0 with OtsuSegmentation bugfix and new MicroServiceIO feature branch.

Best Regards!!!
Ingmar

2D3DPointSetGeometryBug.jpg (953×893 px, 162 KB)

Event Timeline

We couldn't reproduce the visualisation bug.

If we use the PointBasedRegistration the PointSets are displayed correct in the 2d and the 3d visualisation.

We validated that the transformation of the geometry is NOT saved.

As a first guess, this is because the geometry is not saved with the PointSet and the coordinates of the points are not transformed when saved. But we have to do more research.

New remote branch pushed: bug-16115-UseGeometryinPointSetWriter

The problem is indeed the PointSetWriter which does not use the geometry of the given PointSet.

I haven't merged the solution, since it seems that some people would like to have the geometry saved instead the changed pointset.

Before merging we should discuss if we want to save the geometry and therefore change the file format or if we want to save the transformed points, so keep the file format but are not able to reproduce the object correctly.

In my opinion we should stay as close as possible to ITK.
In ITK there is no geometry in Pointsets.
Therefor i would change the points before saving instead of saving an additionaly geometry.

Also, I can not think of an example, where having the geometry separatly saved is useful.

Hi Michael and Bastian,
yes, we also discussed that way.
Having one geometry per pointset in place for faster overall manipulation in the 3d world is helpfull but when it comes to filter algorithms it is error prone in this case when the developer might only concentrate onto the points in the itk pointset within mitkPointSet and might forget about the geometry. This can be prohibited by an apropriate api (as is) but when it comes to saving the final points incorporating the positions and the geometry have to be written. I don't see a benefit to add a geometry section in the xml pointset format. The stored positions act as result. I don't see a need to reproduce working steps. The geometry would also only store one transformation and not changes over time.
Are there other opinions on this?
;) Take care and thanks for the great work,
Ingmar

Since there has been no update since a year and we recently discussed the topic on mitk-users [1], I'll take this ticket and propose a solution.

[1] http://sourceforge.net/p/mitk/mailman/message/34364009/

I'll re-request core-modification flag once I got something.

User maleike has pushed new remote branch:

bug-16115-store-geometry-in-pointset-file

I just pushed branch bug-16115-store-geometry-in-pointset-file with the following changes:

  • the unit test was enhanced to
    1. verify geometry equalness after reading
    2. construct a non-identity geometry before writing
  • reader and writer were enhanced
    • to add a geometry tag per time step
    • read this geometry information and apply it properly
    • the writer now also adds more precision to numbers, as discussed in T177675
  • since there is 100% overlap regarding serialization of a Geometry3D to XML with but 19208, I moved Geometry<-->XML code into a class of its own that could be reused in T19208 as well

Requesting a Needs_Core_Change flag to merge this.
Also marking this for 2015.05.2 in case this is relevant.

This change would resolve all issued mentioned in the original bug report. The visual deformation should be solved via T19241.

(In reply to Daniel Maleike from comment #11)

  • the writer was changed to TinyXML

To add one detail: the file structure itself has been preserved. That means that the writer is a safe replacement for the old writer, not breaking compatibility with any old files or readers.

[6b9b4e]: Merge branch 'bug-16115-store-geometry-in-pointset-file'

Merged commits:

2015-08-17 11:11:58 Daniel Maleike [96991a]
Read geometry information in PointSetReaderService


2015-08-17 11:11:34 Daniel Maleike [926742]
Change PointSet writer to TinyXML structure, add geometry information


2015-08-17 11:08:25 Daniel Maleike [d46dfb]
Add class to serialize/deserialize Geometry3D information


2015-08-17 11:07:18 Daniel Maleike [ad5c20]
Enhance test to 1. verify geometry 2. use non-identity geometry

[ed3853]: COMP: Merge branch 'bug-16115-store-geometry-in-pointset-file'

Merged commits:

2015-08-18 16:20:47 Daniel Maleike [ad5ebc]
FIX: expand time geometry in any case, also when file does not contain geometry

[da5954]: COMP: Merge branch 'bug-16115-store-geometry-in-pointset-file'

Merged commits:

2015-08-18 17:15:21 Daniel Maleike [e9a55c]
Deactivate regression test until fixed

[7073eb]: Merge branch 'bug-16115-store-geometry-in-pointset-file'

Merged commits:

2015-08-18 18:27:01 Daniel Maleike [0f349c]
Make TinyXML write to file in order to get new-lines (readability)


2015-08-18 18:25:46 Daniel Maleike [84d25e]
Reformulate test: create our own reference under the "C" locale

Regression tests versus the "old" file format exist elsewhere,
e.g. PointSetReaderTest, so we do not need to refer to old
files.

While publishing my changes, I missed re-executing all of the tests (my fault). I fixed them by now. The only one that needed real changes was the PointSetLocaleTest. This test was testing two aspects even if the name would only mention the locale:

  • unchanged file format under different locales, "C" as reference
  • unchanged file format regarding an older reference file

Since the "new" writer adds geometry information, the locale test would fail, noticing that the new files would have too much XML in them.

As we already have a PointSetReaderTest that verifies that we can still correctly read the original/old file format, I changed the PointSetLocale test's definition. It now creates its own reference file using the "C" locale, and then executes the test, switching between multiple locales, each time verifying that the generated output file is identical to the "C" reference byte by byte.

[f1c226]: Merge branch 'bug-16115-store-geometry-in-pointset-file'

Merged commits:

2015-08-18 18:48:11 Daniel Maleike [4fa2c8]
Fix warning

[81623a]: COMP: Merge branch 'bug-16115-store-geometry-in-pointset-file'

Merged commits:

2015-08-18 19:21:45 Daniel Maleike [4e4cfc]
Redefining interface (one compiler does not like it otherwise)