MRtrix3 / mrtrix3

MRtrix3 provides a set of tools to perform various advanced diffusion MRI analyses, including constrained spherical deconvolution (CSD), probabilistic tractography, track-density imaging, and apparent fibre density
http://www.mrtrix.org
Mozilla Public License 2.0
290 stars 178 forks source link

MRview: Lightbox mode #29

Closed Lestropie closed 9 years ago

Lestropie commented 10 years ago

To show several slices at once of the same axis. Could be useful take a quick screen shot of results for meetings etc.

Features could include:

rtabbara commented 9 years ago

Initial commit for LightBox mode can be found in the light_box branch so please take a look. A few things:

jdtournier commented 9 years ago

Hi Rami,

Great work, it looks really good. To answer your questions:

Just one thing I've noticed: the current setup allows for negative slice spacing. Maybe that should be prevented...? Or should we consider that a feature...? In two minds about that at the moment...

Cheers, Donald

draffelt commented 9 years ago

Regarding the first point, Rami did originally create a thin mode-dependent horizonal menu at the top, that contained the lightbox specific configuration (then I told him to stick it in the view menu). The main problem was that it took up valuable vertical real estate, and could not be collapsed.

I wonder if it's worth having the view tool box as a 'view menu' that is more permanent (but collapsible) on the left hand side?

Another thing I'm in two minds about is to have a list of loaded images as a panel in the view menu (bit like vector and overlay plot currently). The benefit of this would be that you can set the windowing on multiple images simultaneously. It's probably something done rarely, so not that important. I know you can mrcat the images together first, but most users would not think of this.

On Mon, Feb 23, 2015 at 11:03 PM, J-Donald Tournier < notifications@github.com> wrote:

Hi Rami,

Great work, it looks really good. To answer your questions:

-

Yes, that's the best place at the moment.

I'd like to figure out a better strategy long term for some of these options, it's not obvious where to find them if you haven't used them before. This goes for the threshold, transparency & clip planes controls for the render mode, and the grid controls in this new lightbox mode. I'd like to make them more obvious without cluttering up the UI, and also while keeping things as simple as possible in terms of the code. At the moment, modes are kept distinct from tools, and we try to keep it that way to ensure all tools can be used with all modes as much as possible without having to start handling all manner of edge cases. I'm open to suggestions about the cleanest way to expose these controls...

Agree. I tend to use the cube root of the product of the voxel sizes for this. This will be important when loading for example non-human data (e.g. mouse) where the defaults simply don't make sense.

Yes, this may seem a bit odd, along with a few other related things. This indeed doesn't make much sense until you try to overlay other data onto the slices, and these data are interpolated or at least projected onto the slice at the finer resolution. I've often found it annoying in the previous version of MRtrix that I was stuck at the default 1 voxel increment even though the tracks I was displaying were cropped to a thinner slice - it made it impossible to navigate the tracks in a continuous, fine-grained manner. So I've purposely designed this version to allow fine-grained control of the actual focus & target even if that makes no difference to the slice currently displayed.

Bear in mind also that this behaviour only occurs when the axes are locked. As soon as any image tilt is allowed, the data are loaded in a 3D texture and resliced using trilinear interpolation on the GPU, so the images displayed will differ even though the spacing is smaller than a voxel. Also, any images overlaid with the overlay tool will use this strategy, even if the "lock to axes" box is ticked. This might be important for instance if the main image has a coarser resolution than the overlaid image: if you restricted spacing to the main image's voxel spacing, this would prevent display of the overlay at its genuine resolution. Basically, you can think of lots of situations where similar considerations will crop up.

Just one thing I've noticed: the current setup allows for negative slice spacing. Maybe that should be prevented...? Or should we consider that a feature...? In two minds about that at the moment...

Cheers, Donald

— Reply to this email directly or view it on GitHub https://github.com/jdtournier/mrtrix3/issues/29#issuecomment-75530332.

draffelt commented 9 years ago

I'm in favour of the negative slice increment.

On Tue, Feb 24, 2015 at 9:04 AM, David Raffelt draffelt@gmail.com wrote:

Regarding the first point, Rami did originally create a thin mode-dependent horizonal menu at the top, that contained the lightbox specific configuration (then I told him to stick it in the view menu). The main problem was that it took up valuable vertical real estate, and could not be collapsed.

I wonder if it's worth having the view tool box as a 'view menu' that is more permanent (but collapsible) on the left hand side?

Another thing I'm in two minds about is to have a list of loaded images as a panel in the view menu (bit like vector and overlay plot currently). The benefit of this would be that you can set the windowing on multiple images simultaneously. It's probably something done rarely, so not that important. I know you can mrcat the images together first, but most users would not think of this.

On Mon, Feb 23, 2015 at 11:03 PM, J-Donald Tournier < notifications@github.com> wrote:

Hi Rami,

Great work, it looks really good. To answer your questions:

-

Yes, that's the best place at the moment.

I'd like to figure out a better strategy long term for some of these options, it's not obvious where to find them if you haven't used them before. This goes for the threshold, transparency & clip planes controls for the render mode, and the grid controls in this new lightbox mode. I'd like to make them more obvious without cluttering up the UI, and also while keeping things as simple as possible in terms of the code. At the moment, modes are kept distinct from tools, and we try to keep it that way to ensure all tools can be used with all modes as much as possible without having to start handling all manner of edge cases. I'm open to suggestions about the cleanest way to expose these controls...

Agree. I tend to use the cube root of the product of the voxel sizes for this. This will be important when loading for example non-human data (e.g. mouse) where the defaults simply don't make sense.

Yes, this may seem a bit odd, along with a few other related things. This indeed doesn't make much sense until you try to overlay other data onto the slices, and these data are interpolated or at least projected onto the slice at the finer resolution. I've often found it annoying in the previous version of MRtrix that I was stuck at the default 1 voxel increment even though the tracks I was displaying were cropped to a thinner slice - it made it impossible to navigate the tracks in a continuous, fine-grained manner. So I've purposely designed this version to allow fine-grained control of the actual focus & target even if that makes no difference to the slice currently displayed.

Bear in mind also that this behaviour only occurs when the axes are locked. As soon as any image tilt is allowed, the data are loaded in a 3D texture and resliced using trilinear interpolation on the GPU, so the images displayed will differ even though the spacing is smaller than a voxel. Also, any images overlaid with the overlay tool will use this strategy, even if the "lock to axes" box is ticked. This might be important for instance if the main image has a coarser resolution than the overlaid image: if you restricted spacing to the main image's voxel spacing, this would prevent display of the overlay at its genuine resolution. Basically, you can think of lots of situations where similar considerations will crop up.

Just one thing I've noticed: the current setup allows for negative slice spacing. Maybe that should be prevented...? Or should we consider that a feature...? In two minds about that at the moment...

Cheers, Donald

— Reply to this email directly or view it on GitHub https://github.com/jdtournier/mrtrix3/issues/29#issuecomment-75530332.

jdtournier commented 9 years ago

Not sure GitHub's reply-by-email feature is production-ready yet...

Anyway, I reckon these are all great ideas. The view menu is special enough that it could be handled separately, and have its own collapsible sidebar, maybe on the left hand side. And bringing the image list in there is also a good idea, I've actually used the window range controls to do just what you suggested...

Happy to keep the negative increments. Anyone else have an opinion on the matter...?

Lestropie commented 9 years ago

One option would be to have a dedicated 'mode-specific options' toolbar, that would contain only options that are applicable to specific view modes, e.g. clip planes for volume render, gridding for light box. Not sure if it actually solves any problems though.

The view options can be manually dragged away from the other tool menus as it is, maybe it just needs some default parameter tweaking so that by default it will open on whichever side does not currently have any tools docked, and the user can always drag it onto the other tools to merge with them?

We also discussed having the mode-specific options at the bottom of the the view toolbar disappear completely if that mode is not currently active.

If there's tool bar real estate, my preference would be to have a 'reverse order' tick box that would perform identically to a negative slice increment, and then limit the actual slice increment to be non-negative. I can see users being taken aback and reporting the current behaviour. It would also make it more consistent with other comparable controls e.g. slab thickness in the tractography tool.

jdtournier commented 9 years ago

One option would be to have a dedicated 'mode-specific options' toolbar, that would contain only options that are applicable to specific view modes, e.g. clip planes for volume render, gridding for light box. Not sure if it actually solves any problems though.

That would be hard to achieve without sacrificing a fair bit of real estate in the top bar. There's quite a few of these controls, and some of them are multi-line - including Dave's suggested image list.

The view options can be manually dragged away from the other tool menus as it is, maybe it just needs some default parameter tweaking so that by default it will open on whichever side does not currently have any tools docked, and the user can always drag it onto the other tools to merge with them?

Yes, I think that's basically what we're talking about. From a coding perspective though, I think the best way to handle this it to have that particular sidebar not be a Tool, since it would require a fair bit of custom handling to get right. I think it's core enough that it should be considered a part of the main UI, rather than an optional add-on (which really is what Tools should be).

We also discussed having the mode-specific options at the bottom of the the view toolbar disappear completely if that mode is not currently active.

Good idea. Should be trivial to handle by calling setVisible() instead of setEnabled().

If there's tool bar real estate, my preference would be to have a 'reverse order' tick box that would perform identically to a negative slice increment, and then limit the actual slice increment to be non-negative. I can see users being taken aback and reporting the current behaviour. It would also make it more consistent with other comparable controls e.g. slab thickness in the tractography tool.

That's why I raised the issue. The difference with the tractography slab width though is that a negative width is genuinely nonsense - there is no interpretation that makes sense. With the negative increments, it is well-defined, and very easy to understand when confronted with it. It's a bit surprising that it should allow this, but it shouldn't take long for users to see that it does make sense and that they might themselves find it useful someday.

And generally, I'm in favour of allowing the broadest set of user input that we can reasonably interpret, since experience (mostly others', not just mine) shows that this allows the application to be used in ways we (the developers) hadn't envisaged. So I have a general bias towards allowing weird behaviours when it is entirely consistent (in terms of the code) with normal operation.

Lestropie commented 9 years ago

That would be hard to achieve without sacrificing a fair bit of real estate in the top bar. There's quite a few of these controls, and some of them are multi-line - including Dave's suggested image list.

I guess I meant its own tool, rather than a tool bar. But if people like the idea of having the relevant mode-specific options appear and disappear, I think it's better as part of the view options.

Not completely against the negative slice increment if others are for it. Though I need to have a play with the AdjustButton, make sure it transitions sensibly from positive to negative numbers on mouse drag. If it's only reasonably accessible by manually entering a negative number, users may not find the functionality, whereas with a dedicated tick box they would.

rtabbara commented 9 years ago

Thanks everyone for the feedback. I've made some small modifications in the light_box branch. As suggested:

Just as an aside, I was noticing sluggish performance when in light box mode for images with a bit higher res and a moderate number of rows and columns. Some profiling revealed that the culprit is within Image::update_texture2D. The issue is that texture2D is recreated after every draw call because these are only caching the plane dimensions.

But as soon as the image is tilted this is no longer an issue because here we're using render3d and loading the full 3d texture. So I'm wondering whether in this mode we should force render3d irrespective of whether or not the axes are locked? I guess as an added bonus this means that we'll get interpolation across slices?

jdtournier commented 9 years ago

Yes, good call. I agree, it would most definitely be a better idea to use the 3D texture here. I guess to achieve this you'll need to provide some way of telling the slice mode function that does the rendering to use the 3D texture - shouldn't be an issue... (?)

rtabbara commented 9 years ago

Ok, finished polishing up the last few things for the light box mode. It's now rendering using the 3d tex, so aside from the initial loading lag, rendering is silky smooth for a high number of rows/cols.

Please let me know what you guys think --- maybe look to merge?

Lestropie commented 9 years ago

Just tested on my Windows box for the sake of it, runs fine, don't think I even noticed the delay on load.

A few things to think about:

draffelt commented 9 years ago

Regarding the above points:

I think once the grid defaults are changed we are good to merge.

Lestropie commented 9 years ago
  1. It was a bit weird. If I can replicate I'll try to either get more specific on the circumstances or find the issue myself. Not a merge-breaker though.
  2. Cool. It should therefore also play John Mayer when you switch to lightbox mode.
  3. Trouble I have with mm is that if you've got a slightly non-BCD voxel size (e.g. 0.892468mm), it becomes hard to set up an integer slice gap. Off-axis will always be weird if not in mm... Another option would be to have two AdjustButtons for slice gap side-by-side, one showing mm and the other showing slices, and disable the second one if not axis-locked so you can only work in mm where the interpretation is unambiguous. If nobody else is fussed I might play with this at some stage, don't let it halt the merge.
rtabbara commented 9 years ago

Merged in 413e422ee2c38945051fef521820719c13e66cbb. Thanks everyone for the input. I think Dave was suggesting we should make users aware of this new mode on the google+ page?

I'm going to close off this issue for now.