There were at least crashes, hierarchy issues, and compatibility problems. Before advancing to improve the current or creating a new implementation, tests should be enhanced. I'll start thinking about this and start coding in a couple of weeks.

User maleike has pushed new remote branch:

bug-18280-sceneio-tests

I noted initial ideas and already created a basic structure for more profound tests. So far I am just beginning to test hierarchy reproduction.

Nice effects:

• T19322 has been reproduced by my tests, integrating its branch made the tests pass again :-)
• (at least) one test case (ComplicatedFamiliySituation) produces a .mitk File that generates a "Unknown error while loading" dialog when loading it into the application (the unit test like it, though!

Necessary enhancements:

• comparing data, incl. geometry
• comparing properties
• (comparing mappers/interactors) - in parentheses because those are not supported by current SceneIO, so we don't yet need to test them

• new class of rendering tests to validate consistent display of "historical scenes"

Update:

I enhanced the prepared test structure which is now capable to compare most relevant aspects of two DataStorages (further ideas noted in comments).

Motivational aspect: some very basic tests that I added were already able to reproduce a number of errors fixed in other bugs (i.e. I needed to merge the corresponding branches):

• T19322
• T16115
• T19208
• T19278

Requesting core change flag for some minor changes that were required in order to allow implementation of tests, or to make them pass:

• DataNode:::GetPropertyListNames() added: Provides the list of all strings that have been used to store PropertyLists (usually specific to renderers) in a DataNode. Since there is a member GetPropertyList(std::string), nobody outside the node can know what strings have been associated by other parts of the program. Thus this method..

• mitk::Equal(GeometryData) added: goes along the pattern of Equal methods for Image, Surface etc.

• mitk::LevelWindow::operator==() modified: Comparing values using an an epsilon now. Due to non-integer float values for bounds etc. that are associated for a default level/window in one test case, I found this the easiest way to make the property compare equal after deserialization.

For easier review, a patch of those changes in MitkCore is attached to this bug.

Changes in core

I had a phone discussion with Caspar and Keno. They principally agree with my changes. Regarding the core changes, Caspar had two remarks that I included in my latest version. I updated the attachment accordingly.

Some comments from my side:

1. Regarding the BaseDataEqual

I am not really happy with this solution. Ideally the BaseDatas should be comparable the same way everything else is via mitk::Equal(BaseData, BaseData, eps, verbose) define in the mitkBaseData.h .

I understand the reasoning, but this way it only works for a few specifically mentioned types. Maybe we need to add an InternalEqual function to the BaseData that needs to be implemented in each derived class.

2. Parent order

std::string mitk::DataStorageCompare::GenerateNodeDescriptor(mitk::DataNode::Pointer node)
and
std::string mitk::DataStorageCompare::GenerateHierarchyDescriptor(mitk::DataNode::Pointer node, mitk::DataStorage::ConstPointer storage)

both create different strings for nodes if everything is identical but the order of parents has changed. Should they really be considered different in that case?

3. Copyright

What is the license for:
http://4.bp.blogspot.com/_57lEz2uo9yw/TTGWNhjsWPI/AAAAAAAAA5M/yMauv61ULXc/s1600/web-technology-family-tree-large.jpg

4. Minor nitpick

I am not that much of a fan of the classname of
mitkMoreSceneIOTest
maybe something more descriptive? Otherwise adding a number at the end would be more in keeping with something like mitkImageCast2.cpp etc.

(In reply to Caspar Jonas Goch from comment #7)

Some comments from my side:

Caspar, thanks for your review. I'll respond in context below.

1. Regarding the BaseDataEqual

I am not really happy with this solution. Ideally the BaseDatas should be
comparable the same way everything else is via mitk::Equal(BaseData,
BaseData, eps, verbose) define in the mitkBaseData.h .

I understand the reasoning, but this way it only works for a few
specifically mentioned types. Maybe we need to add an InternalEqual function
to the BaseData that needs to be implemented in each derived class.

I agree. I cannot implement a solution for all instances of BaseData at the moment, but I renamed BaseDataEqual to BaseDataCompare to make it more apparent that it serves another purpose.

2. Parent order

std::string
mitk::DataStorageCompare::GenerateNodeDescriptor(mitk::DataNode::Pointer
node)
and
std::string
mitk::DataStorageCompare::GenerateHierarchyDescriptor(mitk::DataNode::
Pointer node, mitk::DataStorage::ConstPointer storage)

both create different strings for nodes if everything is identical but the
order of parents has changed. Should they really be considered different in
that case?

Good catch, I did not see that. I also consider parent order irrelevant at the moment. That could make the subject of some MITK meeting discussion if you'd like. I adapted the code to sort parent strings now.

3. Copyright

What is the license for:
http://4.bp.blogspot.com/_57lEz2uo9yw/TTGWNhjsWPI/AAAAAAAAA5M/yMauv61ULXc/
s1600/web-technology-family-tree-large.jpg

No idea about the rights, nothing was marked in the blog of origin. I removed the link and replaced the hierarchy by something irrelevant and unrelated.

4. Minor nitpick

I am not that much of a fan of the classname of
mitkMoreSceneIOTest
maybe something more descriptive? Otherwise adding a number at the end would
be more in keeping with something like mitkImageCast2.cpp etc.

You are right, I chose the name in lack of better inspiration and in order to not yet replace the old test. I renamed it to mitkSceneIOTest2 now.

[f04eed]: Merge branch 'bug-18280-sceneio-tests'

Merged commits:

2015-10-09 17:35:09 Daniel Maleike [794574]
Replace Hierarchy w/ unknown copyright by something made up

2015-10-09 17:34:48 Daniel Maleike [e3b509]
Sort hierarchy descriptors to avoid random order

2015-10-09 17:34:12 Daniel Maleike [249cea]
Rename files (content already renamed)

2015-10-09 17:31:15 Daniel Maleike [e8f21c]
Rename BaseDataEqual --> BaseDataCompare

2015-10-09 17:30:28 Daniel Maleike [7450b0]
Add rendering test data, but deactivate tests (failing)

2015-10-09 17:30:01 Daniel Maleike [c61f90]
Rename test suite

2015-10-09 16:52:43 Daniel Maleike [98a430]
Rename MoreSceneIOTest --> SceneIOTest2

2015-10-07 16:51:25 Daniel Maleike [e17b25]
Add rendering test (line width for Surface changed int to float)

2015-10-07 12:29:59 Daniel Maleike [458a5d]
Add rendering test (rainbow colored CTs from old scene files)

2015-10-07 14:57:44 Daniel Maleike [140847]
Improve documentation and readability

Thanks to Caspar for the feedback

2015-10-06 17:29:25 Daniel Maleike [852ab8]
Hints towards precision limits

2015-10-06 17:10:40 Daniel Maleike [c17211]
Fix remaining gcc compile issues

2015-10-06 16:43:14 Daniel Maleike [153d95]
First set of fixes for gcc

2015-10-06 15:59:57 Daniel Maleike [ea6fe7]
Merge branch 'bug-19208-geometrydata-sceneio-hook-safe' into bug-18280-sceneio-tests

2015-10-06 15:20:16 Daniel Maleike [86a372]
Enhance documentation

2015-10-06 14:22:55 Daniel Maleike [200ee3]
Make SceneIO save all PropertyLists independent of renderers.

2015-10-06 12:48:18 Daniel Maleike [9d48e2]
Add test scenario for property lists, fails!

Should mostly be tested in mitkPropertySerializationTest,
but the aspect of multiple lists is specific to SceneIO

2015-10-06 12:47:02 Daniel Maleike [f20b18]
Add test for mappers, update comments

2015-10-06 11:44:26 Daniel Maleike [57d71b]
Implement PropertyList comparison

2015-10-06 11:44:00 Daniel Maleike [245828]
Read written double values as double (not as float)

2015-10-06 11:43:09 Daniel Maleike [561e8f]
More generous operator== for LevelWindow

(Test for serialization of this property would
get much more complicated otherwise)

2015-10-06 11:41:45 Daniel Maleike [81e8cb]
Add DataNode::GetPropertyListNames()

In order to learn about all possible PropertyLists in situations
where this cannot be learnt from the available render windows
(e.g. SceneIO tests)

2015-10-06 09:39:36 Daniel Maleike [5d1ab9]
Restructure comparisons to reuse/centralize node matching logic

2015-10-05 18:35:53 Daniel Maleike [503659]
Split BasicCoreTypes() into multiple methods

2015-10-05 18:31:38 Daniel Maleike [10761a]
Add precision parameter to scenarios

2015-10-05 18:05:39 Daniel Maleike [bdc437]
Merge branch 'bug-19278-use-eps-in-equal-transformtype' into bug-18280-sceneio-tests

2015-10-05 18:04:44 Daniel Maleike [e50908]
mitk:Equal for mitk::GeometryData

2015-10-05 17:31:21 Daniel Maleike [8b1c7a]
Merge branch 'bug-19208-geometrydata-sceneio-hook-safe' into bug-18280-sceneio-tests

> test for GeometryData should pass now

2015-10-05 17:30:43 Daniel Maleike [2ce122]
Add GeometryData test object (test fails!)

2015-10-05 17:24:15 Daniel Maleike [d58f76]
Basic surface

2015-10-05 17:06:29 Daniel Maleike [cf31a8]
Merge branch 'bug-16115-store-geometry-in-pointset-file' into bug-18280-sceneio-tests

> fix test that detected above bug

2015-10-05 17:03:19 Daniel Maleike [fb4a78]
Activate a test case for PointSet - fails due to geometry

2015-10-05 17:00:38 Daniel Maleike [33a018]
Add structure to compare BaseData contents - provide implementations for Surface/Image/PointSet

2015-09-18 17:51:45 Daniel Maleike [c1debe]
Adding a more complex hierarchy to test scenarios

2015-09-18 16:51:50 Daniel Maleike [ef16e4]
Merge branch 'bug-19322-load-complex-scenes' into bug-18280-sceneio-tests

2015-09-18 16:51:05 Daniel Maleike [aabd60]
Initial ideas about comparing two data storages

2015-09-18 12:34:20 Daniel Maleike [c9447d]
Write up some test ideas, build basic I/O test structure (writing)

[f4a51a]: Merge branch 'bug-18280-sceneio-tests'

Merged commits:

2015-10-09 19:24:46 Daniel Maleike [17b463]
COMP: Fix warning (Windows), deactivate one test case on 32 bit systems

[33a519]: Merge branch 'bug-18280-sceneio-tests'

Merged commits:

2015-10-09 19:41:33 Daniel Maleike [4d763a]
Output number of tests (too see differences on passing dartclients)

[42c79a]: Merge branch 'bug-18280-sceneio-tests'

Merged commits:

2015-10-09 19:05:42 Daniel Maleike [dec440]
Activate test case

I implemented a first portion of tests but need to fix some problems before this can be considered done.

Remaining:

• rendering test for T19354: I wanted a size of 120x120, but seemingly most darclient platforms enforce a bigger size. I need to re-generate reference images, update MITK-Data
• rendering test for T19360: fix awaiting core-change-request, plus probably the same issue as above
• precision problem for geometry serialization: the Linux 32 bit client generated an error for a test case that serializes a GeometryData node. Other GeometryData-I/O tests are passing on that platform, test is also passing on Windows (which seems to be 32 bit??).

[85a379]: COMP: Merge branch 'bug-18280-sceneio-tests'

Merged commits:

2015-10-09 20:45:46 Daniel Maleike [ad0364]
Deactivate test (failing because of size problems), increase test size to 150x150px

[b4977a]: Merge branch 'bug-18280-sceneio-tests'

Merged commits:

2015-10-12 09:35:03 Daniel Maleike [4c0679]
Merge branch 'bug-19354-ct-image-no-rainbow' into bug-18280-sceneio-tests

2015-10-12 09:32:46 Daniel Maleike [d19829]
Activate test at 200x200px

(In reply to Daniel Maleike from comment #13)

I implemented a first portion of tests but need to fix some problems before
this can be considered done.

Remaining:

• rendering test for T19354: I wanted a size of 120x120, but seemingly most darclient platforms enforce a bigger size. I need to re-generate reference images, update MITK-Data

Done, now integrated and passing