Page MenuHomePhabricator

Compatibility with old scene files broken (Images in rainbow colors after suppression of LevelWindow_Color rendering mode)
Closed, ResolvedPublic

Description

Report:

There is a rendering issue when loading older scene files (probably from before T17547, 04/2014) into a current Workbench. CT images that should be displayed with a black-to-white lookup-table are displayed in all colors of the rainbow (see original report on mitk-users quoted at the end of this description).

Analysis:

Digging into this problem shows that those images would have two aspects that mess up now:

  • they refer to a rendering mode LevelWindow_Color which has been removed in T17547
  • they do not have a lookup table, which was not required before either named bug or before a different rendering change

Proposed solution:

I found the easiest and probably most reasonable solution as follows:

  • SceneReaderV1 (the class that reads a .mitk file and constructs DataNodes and properties) will by default remove all default properties from a newly created DataNode after it calls SetData. This is well justified behavior (explained below).
  • We could and should add local exceptions to this behavior to handle compatibility issues like above. In the specific case, we need to keep the default lookup table that is generated, which happens to be a reasonable one for CT images (black-to-white)

Testing:

I'll provide a .mitk scene file that demonstrates this behavior. This allows manual testing. However, I'll also try to set up an automatic test that loads this file and verifies correct rendering (relation to T18280).

Explanation on suppressing default properties in scene loader:

We want to load scenes that are the same as before. There can
be two interpretations of this:

  1. data is restored correctly
  2. visual appearance is restored correctly.
  1. would be exactly achieved if we remove all new default properties

and only load those properties that existed when saving the scene.
This would avoid new properties "appearing".

One could argue that the minority of applications cares for the exact
set of properties. But here comes

  1. sometimes properties are changed in their type (PointSet's or Surface's

"line width" from int to float). The mapper implements a reasonable
fallback behavior (use new default type, if not present, look for
the old one). If now we would have a scene loader that both keeps
default properties (e.g. line width 1.5) and adds loaded properties
(e.g. line width 10), we would have another backwards compatibility
problem.

Quoting Ingmar Wegner on mitk-users (25/09/15), also mentioned in T19294:

I just debugged into the serialization because all images within
our project files that we created before moving to the 2015.05.0
release show up in rainbow color, not in gray values.

I found out that there has been a change in the serialization.
Now there is an additional property "LookupTable" in the
properties file.
During loading an mitk file the default behavior is to add a
rainbow LUT. I guess that is to support other image types from
the diffusion application?
But as the "old" project files don't contain the property the
images get also drawn in rainbow color.
Then if you go into the DataManager plugin and want to reset
the colormap it automatically switches to "grayscale" once the
context menu opens up.

Event Timeline

Attached an example for testing.

User maleike has pushed new remote branch:

bug-19354-ct-image-no-rainbow

I pushed the proposed fix to achieve backwards compatibility.

Locally I also added a rendering test to verify behavior. I'd prefer to integrate this test with T18280, however. This bug adds quite some tests for SceneIO and it is easier to touch CMake files and MITK-Data only there.

Can you somehow guarantee that you didn't break compatibility at another place with your fix? At a glance, this seems to be a critical change that has to be thoroughly thought through.

It does look good to me. Also the change in this branch is not that massive.
It basically replaces the
propertyList->Clear();
with a new
ClearNodePropertyListWithExceptions(*node, *propertyList);

which also does the Clear() and only re-adds the kept properties (so far only the LUT-property and that only if it is an mitk::Image)

Behaviour for newer scene files should be unchanged as the serialized property list is concatenated with "replace = true" lateron.

(In reply to Stefan Kislinskiy from comment #4)

Can you somehow guarantee that you didn't break compatibility at another
place with your fix? At a glance, this seems to be a critical change that
has to be thoroughly thought through.

The fix changes behavior in a single case: when a (mostly older) scene file contains an mitk::Image that has no "LookupTable" (LUT) property. (newer Images stored in DataNodes get such a property by default). For such images, we let them get the new default LUT from the associated mapper. This solves named compatibility problem.

Other cases:

  • a scene alreay contains a "LookupTable" property: this stored value will still replace the original value (use of ReplaceProperty() calls in SceneIO). This keeps regular loading working.
  • a DataNode where somebody in a current mitkWorkbench removes the "LookupTable"-property, saves the scene, then restores it. In this case we would restore a default value that had been removed.
    • a rather unprobable and unhealthy use case?
    • I have no idea if this is possible to do and how the mappers would react. But indeed, this is a case to look at. I'll do so before integrating the change.

(In reply to Daniel Maleike from comment #6)

(In reply to Stefan Kislinskiy from comment #4)

Can you somehow guarantee that you didn't break compatibility at another
place with your fix? At a glance, this seems to be a critical change that
has to be thoroughly thought through.

  • a DataNode where somebody in a current mitkWorkbench removes the

"LookupTable"-property, saves the scene, then restores it. In this case we
would restore a default value that had been removed.

  • a rather unprobable and unhealthy use case?
  • I have no idea if this is possible to do and how the mappers would

react. But indeed, this is a case to look at. I'll do so before integrating
the change.

I tested this case. In fact the following happens:

  • the renderer does not crash, but fall back to an internal rainbow LUT (as happened for the scenes described in this bug)
  • saving/restoring the scene would reset the LookupTable property to black-to-white

I thought a little about this problem, but I'm keeping with my solution. In my opinion nobody should demand evaluation or a LUT via the corresponding RenderingMode property and then not provide a LUT. I understand the rainbow colors as a "warning" in this case.

I agree with the reasoning from comment 7. Manually deleting the LUT property to force default-default behaviour is... not elegant and should not be catered to.

[cc840c]: Merge branch 'bug-19354-ct-image-no-rainbow'

Merged commits:

2015-10-07 10:55:33 Daniel Maleike [ed1882]
Add method to selectively keep default properties for compatibility.

Detailed description in T17547