Page MenuHomePhabricator

[LevelWindowManager] Different behavior in AutoTopMost case and fallback case
Open, NormalPublic

Description

mitk::LevelWindowManager::Update checks if the AutoTopMost mode is enabled. If so, it continues within SetAutoTopMostImage. If not, it continues within the update-function itself.
In the update-function itself there are some checks (e.g. node visibility). The most important check is the check for the "imageForLevelWindow"-property. If this property is set, a node is set for the level window. If no such node is found, the function uses the topmost visible node as fallback.

In the SetAutoTopMostImage-function the check for the "imageForLevelWindow"-property is not done. This makes sense, as this property is not relevant (it is even removed for each node). What is important is the topmost visible node (hence, the name AutoTopMost).
However, the SetAutoTopMostImage-function additionally uses mitk::LevelWindowManager::IgnoreNode to ignore each node that has either the RenderingModeProperty::LOOKUPTABLE_COLOR or RenderingModeProperty::COLORTRANSFERFUNCTION_COLOR set.
This function is NOT used inside the update-function itself, which means that - if the AutoTopMost mode is disabled, the level window widget is visible for a node with one of those two rendering mode properties set.

To reproduce:

  1. start MITK workbench, load a dataset
  2. see the LevelWindowSlider on the right side of the Standard Display
  3. open the Properties Plugin view and select the loaded node
  4. change the property Image Rendering -> Mode to either LookupTable_Color or ColorTransferFunction_Color
  5. see how the LevelWindowSlider disappears
  6. change the property Image Rendering -> Mode back to either LookupTable_LevelWindow_Color or ColorTransferFunction_LevelWindow_Color
  7. see how the LevelWindowSlider appears
  8. open the context-menu on the LevelWindowSlider and disable Images -> Set topmost image
  9. change the property Image Rendering -> Mode to either LookupTable_Color or ColorTransferFunction_Color
  10. see how the LevelWindowSlider DOES NOT disappear
  11. additionally you can move / change the LevelWindowSlider and see that the rendered image does not change

My suggestion when fixing this is to also simplify the different checks by introducing a function that checks these different conditions, possibly using some node predicates or lambdas or the like. Redundancy should be removed!

Event Timeline

kalali triaged this task as Unbreak Now! priority.Jan 29 2021, 8:17 PM
kalali created this task.
kalali updated the task description. (Show Details)

While I think I understand the bug, it seems a little artificial in reproduction and high in priority. Can this be triggered in a real-world scenario without the Properties View? If not, I would vote to postpone a fix after release to have more resources for more prominent bugs i. e. related to segmentation. In particular refactoring in general like mentioned in the last paragraph is something that is prone to introduce new side effects that we are not able to detect in time with only a few days left, even though I see the benefits outside of crunch times. :)

I don't know what the real world scenario is to change the Image Rendering Mode, but basically you just have to perform step 8 and 9 to see the bug (step 10).
I could imagine the Image Rendering Mode being changed to either LookupTable_Color or ColorTransferFunction_Color by a line of code inside a module or plugin, which would then make this bug appear, if the auto topmost mode was disabled.

The fix is probably easy but I'm fine with postponing it because the many level window related classes could need an update.

kislinsk lowered the priority of this task from Unbreak Now! to Normal.Feb 2 2021, 1:12 PM
kislinsk edited projects, added MITK, Next Milestone; removed MITK (v2021.02).
kalali removed kalali as the assignee of this task.Feb 2 2021, 1:38 PM