Rendering component handover.

I have no clue whatsoever why there is a parent node in that mapper. Could be historically related to segmentations, which could need a parent. Should be very easy to fix.

@Taylor: Did you do any further research or do you have a patch already?

I looked into the why question. It seems the reason for the parent node is because the mapper that renders the PlaneGeometryData nodes as crosshairs needs a fast way to know about the other PlaneGeometryData nodes in the scene in order to paint the crosshair lines with a gap at the point where the planes intersect with each other.

I never implemented a solution, but my first thought was maybe a caching solution: on the very first render, search through the DataStorage for any other PlaneGeometryData nodes (potentially slow if there are many nodes in the DataStorage). Cache the results, and keep the cache up to date using observers on the DataStorage that watch for the addition of any new PlaneGeometryData nodes. Or maybe the search through the DataStorage is fast enough even when the DataStorage has many nodes? If timing experiments show that this is the case, then the fix becomes much simpler.

Ok, so the parent node is actually required? That sounds not so easy to fix to me then...

Well, some mechanism for knowing about other visible PlaneGeometryData nodes is needed. Perhaps Daniel Maleike wasn't aware of this when he made a TODO note in the QmlMitkFourRenderWindowWidget.cpp about this issue, but I still think there is a more flexible solution that doesn't sacrifice performance.

Here's a pull request that implements a very simple mechanism for knowing about other visible PlaneGeometryData nodes.

https://github.com/MITK/MITK/pull/90

The changes are split into two commits:

Part 1 is the minimal change to current implementation that to fallback to rendering lines without the an intersection gap if SetDatastorageAndGeometryBaseNode(...) is never called.

Part 2 completely eliminates the need for SetDatastorageAndGeometryBaseNode(...) method. Other instances of PlaneGeometryDataMapper2D are automatically tracked internally using std::set<Self*> s_AllInstances static member so that a gap in the line can be drawn at the point where other visible PlaneGeometryDatas intersect.

Great! I will have a look.

User kilgus has pushed new remote branch:

bug-16765-PlaneGeometryRequiresParentNode

I had a look into the current branch. For me, everything seems legit, although i'm not an expert for this part of the code.

Thanks for reviewing, Michael. Is there someone else who needs to review and approve the changes to get this merged into master?

I asked for different reviews, but nobody feels responsible for this old code ;). If anyone gives the release flag, we can merge the changes. If not, we have to wait until the release is over.

Uhoh... sounds like a problem since this code is used by nearly every MITK-based application that exists today (But also all the more reason to make sure it gets a thorough review before merging into master)

Yes, that's the main reason that this is not merged yet.

Stefan will do the final review for this bug and it will be merged after the upcoming release.

I just made a first review of the pull request:

• In a summary: it is an improvement.

• But: As it is a API-breaking change it cannot be merged for the upcoming release. However, methods that were removed must be marked as deprecated for this release.

• Some changes are commented and commit messages are also clear.

• Minor: Some lines of code were added to already long methods that could be refactored easily into small methods called by the long methods.

• Minor: Why a std::set was chosen for keeping all instances of the mapper instead of a std::vector?

The minors do not need to be fixed though.

I just merged the current master into this bug to keep it up-to-date and easy to integrate. I will mark the removed methods as deprecated for the current release. Thank you for the contribution, Taylor. :)

Marked removed method as deprecated to keep the current API unbroken until next release. The originally removed method is now a no-op.

Hi Stephan,

Thanks for the review.

Yes, I agree that the code could be better factored.

std::set was chosen to guarantee that s_AllInstances would not have any duplicates. As it turns out the bookkeeping is pretty simple, so maybe it's not worth the overhead vs a std::vector.

As we simply forgot to merge this bugfix in the last release, I increased the priority.

User kislinsk has pushed new remote branch:

bug-16765-CommonPlaneGeoemetryDataParent

[e95ada]: Merge branch 'bug-16765-CommonPlaneGeoemetryDataParent'

Merged commits:

2015-08-19 16:07:47 Stefan Kislinskiy [711dae]
Merge remote-tracking branch 'nocnokneo/bug-16765-PlaneGeometryDataMapper2D-remove-parentnode-and-datastorage' into bug-16765-CommonPlaneGeoemetryDataParent

Conflicts:
Core/Code/Rendering/mitkPlaneGeometryDataMapper2D.cpp
Core/Code/Rendering/mitkPlaneGeometryDataMapper2D.h
Examples/QtFreeRender/QtFreeRender.cpp
Modules/QmlItems/QmlMitkRenderWindowItem.cpp
Modules/QmlItems/QmlMitkRenderWindowItem.h

2014-09-22 15:26:52 Taylor Braun-Jones [677671]
Fix PlaneGeometryData not rendered if other PlaneGeometryDatas not set, part 2

Second part to fixing the PlaneGeometryDataMapper2D: completely
eliminate the need for SetDatastorageAndGeometryBaseNode(...) method.
Other instances of PlaneGeometryDataMapper2D are automatically tracked
internally using std::set<Self*> s_AllInstances static member so that a
gap in the line can be drawn at the point where other visible
PlaneGeometryDatas intersect.

Signed-off-by: Taylor Braun-Jones <taylor.braun-jones@ge.com>

2014-09-18 17:38:45 Taylor Braun-Jones [537af1]
Fix PlaneGeometryData not rendered if other PlaneGeometryDatas not set, part 1

Finding other plane geometries is used to render a more visually
pleasing gap at the point where the crosshair lines intersect. But if
the other geometries cannot be found (e.g. because the user did not call
SetDatastorageAndGeometryBaseNode) the the crosshair lines should still
be drawn.

Signed-off-by: Taylor Braun-Jones <taylor.braun-jones@ge.com>

2014-09-22 15:21:40 Taylor Braun-Jones [84b446]
Fix missing and incorrect copyright headers in QmlItems

Signed-off-by: Taylor Braun-Jones <taylor.braun-jones@ge.com>