Maniphest T19247

Crosshair rendering crash for small reslice rendering windows
Closed, ResolvedPublic
Actions

Assigned To

None

Authored By

	• goch
	Aug 19 2015, 2:38 PM

Description

The following issue was reported over the mailing list

Hi guys,

first, a simple one. The new crosshair rendering mechanism didn't work correctly for me when I added the geometry for the small reslice rendering window (going along the vessel path).
At first it crashed and after I fixed that - I noticed that the computation results are incorrect in some cases.

I have changed the code to used boost::icl and corrected the intersection rejection approach. Now it works correctly, however if you want to leave boost as an optional dependency, some other interval arithmetic library or own implementation could be used.

Bug:
Patch attached.

0001-Plane-geometry-data-mapper-crash-and-correctness-fix.patch4 KBDownload

Event Timeline

• goch added projects: MITK, Bugzilla.Aug 19 2015, 2:38 PM

We currently do not want to add a boost dependency to the Core.

Rostislav, should I commit only the part fixing the problem and mention you in the commit message or do you want to create/sign-off a new patch just fixing the crash?

Well, I've written my own implementation for interval arithmetic. Applying the following two patches should bring it to the good state without using boost.

patch 1/2

0001-Plane-geometry-data-mapper-crash-and-correctness-fix.patch4 KBDownload

patch 2/2

0002-Own-implementation-of-interval-arithmetic-needed.patch4 KBDownload

Hi Rostislav,

after applying your patches I get compile errors on linux (missing initializer for the "return{} // Interval completely enclosed").
I did some reformating of your code to bring it more in line with our style guide (see [1]).
Additionally, I do not understand your intersectIntervals function. Why does it return an array of intervals? I would expect it to return a single interval (the intersection) of the two given intervals? See [2] for how you implemented it vs how I would expect it to work.

However I would suggest the following:

Provide one small, signed patch which does nothing but fix the crash
A separate patch (could be its own bug). Which provides a mitk::OpenInterval class and uses it to fix the computation issues you have been having. It would also be great if you could specify what the actual issue was.

[1] http://mitk.org/git/?p=MITK.git;a=commitdiff;h=cb9ab80686f62961c8a9d184f13758a10d501b63

[2]
Your implementation:
std::array<IntervalType, 2> IntersectIntervals(const IntervalType& firstInterval, const IntervalType& secondInterval)

{
  assert(secondInterval.GetUpperBoundary() >= firstInterval.GetLowerBoundary() && firstInterval.GetUpperBoundary() >= secondInterval.GetLowerBoundary()); // Non-intersecting intervals should never reach here

  if (secondInterval.GetLowerBoundary() < firstInterval.GetLowerBoundary())
  {
    if (firstInterval.GetUpperBoundary() < secondInterval.GetUpperBoundary())
    {
      return {}; // Interval completely enclosed
    }
    return{ IntervalType{ firstInterval.GetUpperBoundary(), secondInterval.GetUpperBoundary() }, IntervalType{} };
  }

  if (firstInterval.GetUpperBoundary() < secondInterval.GetUpperBoundary())
  {
    return{ IntervalType{ firstInterval.GetLowerBoundary(), secondInterval.GetLowerBoundary() }, IntervalType{} };
  }

  return{ IntervalType{ firstInterval.GetLowerBoundary(), secondInterval.GetLowerBoundary() },
    IntervalType{ secondInterval.GetUpperBoundary(), firstInterval.GetUpperBoundary() } };
}

};

My expectation:

IntervalType IntersectIntervals(const IntervalType& firstInterval, const IntervalType& secondInterval)
{
  if (secondInterval < firstInterval || firstInterval < secondInterval)
  {
    return IntervalType(); // no intersection f(  f)  s(  s) or s(  s) f(  f)
  }

  if (secondInterval.GetLowerBoundary() < firstInterval.GetLowerBoundary())
  {
    if (firstInterval.GetUpperBoundary() < secondInterval.GetUpperBoundary())
    {
      return IntervalType(firstInterval.GetLowerBoundary(), firstInterval.GetUpperBoundary()); // Interval completely enclosed s(  f(  f)  s)
    }
    return IntervalType(firstInterval.GetUpperBoundary(), secondInterval.GetUpperBoundary()); // s(  f( s)  f)
  }

  if (firstInterval.GetUpperBoundary() < secondInterval.GetUpperBoundary())
  {
    return IntervalType(secondInterval.GetLowerBoundary(), firstInterval.GetUpperBoundary()); // f(  s( f)  s)
  }

  return IntervalType(secondInterval.GetLowerBoundary(), secondInterval.GetUpperBoundary()); // completely enclosed f(  s( s)  f)
}

};

Hi Caspar,

sorry, the function is named incorrectly - it should be subtractIntervals or something like that - so the subtraction could be zero, one or two intervals, thus the array. Sorry for this confusion. It is strange that you have a compilation error - I tried a couple of online compilers which use both GCC 4.9.2 and 5.1 to compile stuff and none reported the errors. I think worst case is you would have to replace "return {}" with "return std::array<IntervalType, 2>{}".

Secondly, I cannot provide a separate fix for the crash - the interval arithmetic _is_ the fix for the crash. The incorrect computation I talked about was actually rather an additional functionality than a bug fix. The original code did not take the plane extents into account, i.e., consider the attached image - the blue plane geometry is small, but it still induces gaps in the big planes, which I consider to be undesirable behaviour.

undesired gaps illustration

Do you happen to have example data you could share?

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

Do you happen to have example data you could share?

Do you mean data to reproduce the crash and unwanted gaps? Unfortunately, PlaneGeometryData does not have a serializer, so it's impossible to produce a scene file that would help reproducing the problem. I could try creating a minimal example though if you need one.

This might be a candidate for a rendering test to make certain the issue does not turn up again in the future. Can it be generated easily by using the mitk::ImageGenerator ?

Do you mean creating a rendering test?

Well technically no, I was talking about generating the data which then is rendered in that rendering test.

Basically, would the following work:

Generate some image using the ImageGenerator (does it need any special dimensions/spacing?) and resize/move one of the planes via code to reproduce your problem. Then render the entire thing to
a) verify it does not crash
b) compare it to how it should look like

I created the test and also added some changes to the mapper itself.

The changes are:

the line representing the plane is computed using the PlaneGeometry bounds, not the ReferenceGeometry bounds, making 2D and 3D representations consistent with each other.
the gap is created only when planes actually intersect (my previous code actually didn't work in some cases)

Hope you agree with these.

Rostislav.

Ref image should go into MITK-Data\RenderingTestData\ReferenceScreenshots

patches_and_ref_image.zip95 KBDownload

Your renaming patch works fine if applied after my changes as well. The last thing to do after that is to rename IntersectIntervals to SubtractIntervals.

Added smaller threshold to image comparison function

patches_and_ref_image.zip91 KBDownload

Ok, hopefully the final iteration.
Instead of always using plane geometry itself to determine the bounds, which is definitely different from what it was before, I finally made the version that is consistent with the 3D plane geometry data mapper:

I allowed the geometries without reference geometry to be rendered.
If the reference geometry is present, it will be used to determine the resulting line. Otherwise the plane geometry itself is used.
When testing the intersection for validity, the same approach is used: if the other plane geometry has reference geometry, then the intersection point is tested for being inside it. Otherwise the point is tested against plane geometry bounds.

All best,

Rostislav.

PS: just use the last attachment - it has all the necessary patches. The renaming patch still has to be applied.

iteration 4

patches_and_ref_image.zip84 KBDownload

I will take a look.

User goch has pushed new remote branch:

bug-19247-crosshair-rendering-crash

I applied the renaming commit and did renamed the intersectIntervals function. I also fixed the linux compile issue by returning an array constructed via default constructor.

Additionally I moved the SimpleInterval helper class to an anonymuous namespace to avoid name collisions.

It compiles fine now and starting the workbench/displaying an image works.

However the test fails with a segmentation fault on ubuntu 15.04 (after copying the screenshot into MITK-Data). So I can not say for sure that it works as intended.
I will take a look at the test on Wednesday and probably re-write it to use Cpp Unit instead of the old test macros.

Could you check whether it works for you?
Additionally it would be great if you could add some comments to the SimpleIntervalX helper classes (based on the bug branch state), because as is they are pretty arcane and will leave the next maintainer scratching their head.

Sounds good!

Regarding the test failure. The test works fine for me (otherwise I wouldn't commit it in such a state :)). The only thing I can think of is that I did all the work in debug mode - maybe switching it to release will change something. I will try that. Just FYI: my config is Win 8.1, MSVS2013, Qt 5.4.2.

Comments... Ummm, I will try :) The code is mostly self-documenting in my opinion, but a couple of places might benefit from clarification - e.g. the equal_range and erase-emplace loop. Just a question - what do you mean by "bug branch state"? You mean the state that the code is in my branch now?

If in doubt ask yourself if the average grandmother/-father would understand your code ;-)

I meant that it would be great if you based your patches on the bug branch I pushed.
bug-19247-crosshair-rendering-crash

I added the comments you asked for, hope they will clear some things up for the future maintainer.

I also renamed one variable (intersectionResult -> subtractionResult) and added one comment about the empty object - please take a look at it.

Rostislav.

mitkPlaneGeometryDataMapper2D.cpp27 KBDownload

Also, in release mode the test also passes fine on my machine (actually, on a different one - this time using Win7, the rest being similar).

Rostislav.

I added a commit with your comments and got the test working as is on linux. I will test it some more tomorrow.

Ok. It seems to work with rotated/swiveled planes.

One more issue though. If I set the crosshair gap to 0 it only renders one half of the crosshair.

Could you please make a change - I think it's easier in this case.

I think the proper way around is replace line 313

while ( otherPlanesIt != otherPlanesEnd )

with

if (gapSize != 0) 
  while ( otherPlanesIt != otherPlanesEnd )

Funnily enough, with the SimpleInterval's constructor doing min/max negative values will work as if their absolutes were taken :)

Seems to work now. Requesting core flag:

Changed files:
Modules/Core/include/mitkPlaneGeometryDataMapper2D.h
Modules/Core/src/Rendering/mitkPlaneGeometryDataMapper2D.cpp
Modules/Core/test/CMakeLists.txt
Modules/Core/test/files.cmake

New files:
Modules/Core/test/mitkPlaneGeometryDataMapper2DTest.cpp

What was changed:
The way the gaps for intersecting planes are calculated. Previously there would be a gap even if the planes did not actually intersect, due to one plane being to small (see attachement: undesired gaps illustration )

How is it tested:
A new test was added to check for correct behaviour for small planes. Prior behaviour should be tested by existing rendering tests.

[bd13ff]: Merge branch 'bug-19247-crosshair-rendering-crash'

Merged commits:

2015-08-30 22:13:39 Caspar Goch [0941a4]
render entire crosshair for gap size 0

2015-08-26 19:35:21 Caspar Goch [5a6ea0]
Added Rostislavs comments

2015-08-24 15:15:34 Caspar Goch [05a0bc]
Make sure helper class is only available locally

2015-08-24 15:08:37 Caspar Goch [80ac04]
fix linux compile issue

2015-08-24 15:08:18 Caspar Goch [40f2da]
More renaming

2015-08-20 15:36:15 Caspar Goch [2fcafa]
some renaming

2015-08-23 19:31:27 Rostislav Khlebnikov [78d775]
test for mitkPlaneGeometryDataMapper2D