Page MenuHomePhabricator
Create Task
Maniphest T22870

Crash on segmentation threshold [0, 1[
Closed, ResolvedPublic
Actions

Description

We use "fa image" with intensity in the range [0.0, 1.0], but most of the time, the range is actually [0.0, 1.0[. For example, the image I use has a max intensity of 0.99999964237213135. The problem is that this value is interpreted as a 1.0 by the QDoubleSpinBox, which make this block crashes in BinaryThresholdTool::ITKThresholdingOldBinary:

typename ThresholdFilterType::Pointer filter = ThresholdFilterType::New();
filter->SetInput(originalImage);
filter->SetLowerThreshold(thresholdValue); // 1.0
filter->SetUpperThreshold(m_SensibleMaximumThresholdValue); // 0.999999

because m_SensibleMaximumThresholdValue has the real max value of the image but thresholdValue takes its value from the spinbox.

I already fixed it in our in-house version. I'll send a PR soon.

Related Objects

Event Timeline

goyette claimed this task.May 18 2017, 7:21 PM
goyette created this task.
goyette added a project: Pull Request.May 18 2017, 9:06 PM

PR: https://github.com/MITK/MITK/pull/198

• goch triaged this task as Normal priority.May 22 2017, 5:18 PM
• goch added a subscriber: • goch.

The fix seems very random to me. Instead of just dividing the lower threshold by two , would it not be better to catch and correct the error at the place mentioned above?

Something similar to:

if(thresholdValue > SensibleMaximumThresholdValue)
{
  thresholdValue = SensibleMaximumThresholdValue;
}
kislinsk added a subscriber: kislinsk.May 23 2017, 8:11 AM

I'm with Caspar here. This is a typical floating point precision issue which would preferably be resolved by using something like std::min() and std::max() when assigning lower/upper values.

goyette added a comment.May 23 2017, 3:38 PM

Hum, looking at it right now, I agree that it looks random :) I know that I tried doing the simple way (like goch's snippet) in the "logical" places but it wasn't working. I don't remember why though so I'll try again and keep you updated.

goyette added a comment.May 24 2017, 9:55 PM

A new PR: https://github.com/MITK/MITK/pull/199

I do think this is a better fix, so thank you for complaining :) It does look less random.

This being said, you will probably tell me that the condition is not at the place it should be... I know, I tried! I wanted to put it right after

m_CurrentThresholdValue = (m_SensibleMaximumThresholdValue + m_SensibleMinimumThresholdValue) / 2.0;

as any sane person would do :) but it doesn't work!

The difficulty are 1) the UI slider and spinner should be at half value 2) no useless computation 3) don't change the code too much. Here's what currently happen on master:

SetupPreviewNode
  OnThresholdingIntervalBordersChanged
    OnSpinnerValueChanged
      UpdatePreview (WOULD CRASH)
  OnThresholdingValueChanged
    OnSliderValueChanged
      OnSpinnerValueChanged
        UpdatePreview (NOT AT HALF, IS AT MAX)
    OnSpinnerValueChanged
      UpdatePreview (WOULD CRASH)

In summary, this is the only place I could find that respects my 3 conditions.

• goch added a comment.Sep 29 2017, 9:57 AM

Pushed new branch T22870-fix-threshold-crash.

• goch mentioned this in rMITKcedb7d528d74: Merge remote-tracking branch 'nilgoyette/T22870-WrongThresholdFloatImage' into….Sep 29 2017, 9:57 AM
• goch mentioned this in rMITK5ea478957b6e: Merge branch 'T22870-fix-threshold-crash'.
• goch closed this task as Resolved.Sep 29 2017, 9:58 AM

Thanks for the contribution.

goyette added a comment.EditedDec 21 2017, 9:34 PM

Cool. The menus were impossible to use on OS X. However, I suggest that you reopen this issue. I tested master to check if the other problem that was explained in my 3-05-2017 post was solved. It affects all OSes. From my tests, it's still a problem. It might be considered minor but:

  • it's still a bug
  • I already posted a solution 4 months ago, it might be useful to use it
• goch added a comment.Dec 22 2017, 9:41 AM

@goyette I assume you meant to add this comment to T23248, T22929, T23985 or most likely T22065? I do not see a connection to this task?

goyette added a comment.Dec 22 2017, 2:20 PM

Hahaha, dammit, yes, I meant T22065. Not sure what happened :) I copied my post in the right issue. Thank you.

MITK · MIC · IMSY · DKFZ