Page MenuHomePhabricator

New feature: Control the position and orientation of rendered slices with NavigationData
Closed, ResolvedPublic

Description

A NavigationDataToNavigationDataFilter that takes NavigationData as input and sets the position and, optionally, the orientation of the slice for a user-specified renderer.

Event Timeline

FYI, I've updated the pull request with some fixes/improvements.

Just FYI, I updated again the pull request again with another fix (NPE).

New remote branch pushed: bug-15777-add-NavigationDataSliceVisualization

I've made a few more fixes locally, but also some feature enhancements. I'll need to get approval before I can contribute the feature enhancements. If there's no rush, it might make sense to hold off integrating this.

An unit test for this class is also still missing.

So we will wait for the new fixes until we integrate this branch. Taylor, please tell us if there is a new version.

Hi Taylor, any news about your changes?

We also had a look on your branch at github, but the last commit there is from the beginning of October.

Regards,

Alfred

Hi Alfred,

I just checked against my local changes. That github branch actually has all the latest feature enhancements and fixes. It works great for me but I have not yet had a chance to write any unit tests for the new filter and will not have a chance for at least a couple months. If this doesn't work for you, I don't mind if you want to just close this bug and drop this. Or, if you'd like to take over the bug branch and finish things up with unit tests, feel free :-)

Regards,
Taylor

so we can integrate this branch, the unit test can be added later on. I will merge it soon.

@Alfred are you still working on the integration?

Unfortunately I've not merged it yet because I've been quite busy. So if anyone has time to take this over, please do so.

User wirkert has pushed new remote branch:

bug-15777-v2-add-NavigationDataSliceVisualization

User wirkert has pushed new remote branch:

bug-15777-v3-add-NavigationDataSliceVisualization

Bug ready to merge, dashboard red right now. (v3 branch).

@Seb Dashboard is green, any news?

I actually have several improvements and bug fixes for this filter since I last submit the pull request. I'll get the pull request updated with those changes tomorrow morning. Would it mess up anyone if I rebased, squashed, and force-pushed to the Github pull request branch?

Oh yes... this currently does need a core modification - but it shouldn't. Right now this filter depends on the change that was proposed in T15892 but was (correctly) deemed invalid by Bastian.

Hi Taylor,

yes, it needs a core modification because I carelessly fixed a typo in the geometries...
Can you please merge your changes in the bug-15777-v3-add-NavigationDataSliceVisualization branch and push it onto github so I don't have to do the integration work once again?

Does this mean the version of the filter does not reorient around the correct slices?
In this case I will wait before merging it into the core. Thanks for your help!

That's right - in ObliqueAxial and ObliqueSagittal slice view modes (which, BTW, the updated pull request calls InlineAxial and InlineSagittal to be more consistent with industry conventions) the slicing plane will not be correct. But the fix should not be a big deal, I'll drop the change to SlicedGeometry3D that I have in my MITK fork and get this filter working the right way before updating the github pull request.

Hi Taylor,

any news on your pull request? Otherwise we will skip this feature for the 2014-10 release and wait for your update until we merge it to the master.

Regards,

Alfred

Hi Taylor, I just had a look at all externally reported bugs in our tracker and saw this one. What's the status? Did you fix the problem that was mentioned above?

Hi Taylor,

any news? Can we include the feature for the 2016-03 release?

Regards,

Alfred

Hi Alfred,

I've moved on from this project, but I've contacted the team that is still working on it and has access to the internal MITK fork to see if they can offer an updated patch for this bug. I believe I left things in a state that addressed all the issues mentioned in this thread (except for the unit test) but I can't remember for sure.

Cheers,
Taylor

User wirkert has pushed new remote branch:

bug-15777-v4-add-NavigationDataSliceVisualization

User wirkert has pushed new remote branch:

bug-15777-v5-add-NavigationDataSliceVisualization

Hi Taylor,

I just wanted to merge this into the master. However, I'm not allowed because we have to have an explicit statement that we have the right to (for legal reasons).

Easiest would probably be a signed of commit:
http://mitk.org/wiki/How_to_contribute

Thank you and thank you also for your continuing great work!

Best
Sebastian

Hi Sebastian,

Glad you find the contributions helpful. Thanks the the MITK for all their work!

I've amended the commit in pull request #19 with a Signed-of-by line:

https://github.com/MITK/MITK/pull/19/commits/4142204f5d7296b1df22a40b761268203ef93c38

Best,
Taylor

wirkert added a subscriber: wirkert.
kislinsk claimed this task.
kislinsk moved this task from Backlog to Suggested on the MITK (2016-11) board.