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
295 stars 182 forks source link

mrview Connectome tool: Streamtubes not displaying #1330

Open Lestropie opened 6 years ago

Lestropie commented 6 years ago

A continuation of #1311; re-listing as the two bugs originally reported appear to in fact be unrelated, and so need a "clean sheet" to investigate the second.

Primarily reported on the forum here, but also mentioned during user testing of the node mesh visualisation problem here.

Attempting to visualise edges as streamtubes within the mrview Connectome tool results in either no edges being displayed, or an outright mrview crash. May be specific to Max OSX.

If any devs can reproduce this it would be greatly appreciated.

LucSam commented 6 years ago

Hi, I just run the debugger on one of our macs and revceived following error.

Edit: The error occurred after changing from streamlines to to streamtubes:

LLDB.txt

(Edit: Uploaded LLDB output as text file for preservation of sanity - Rob)

Lestropie commented 6 years ago

Thanks for the input @LucSam.

Okay, that's ... awkward. The problematic line is here: specifically statement "assert (streamtube);" fails. Data for this class should be allocated by the following stack:

Since this all looks as it should, I can only suggest that this must be some sort of GL update timing problem with Macs: Something is invoking updateGL() before function Connectome::get_streamtubes() has completed, and hence the data required for that graphical update have not yet been allocated. @jdtournier Would you agree? Though I'd have thought that such behaviour would be causing more widespread issues in mrview given our extensive use of resource-allocation-on-demand.

@LucSam: One thing you could try is replacing file src/gui/mrview/tool/connectome/edge.h, line 60:

void render_streamtube() const { assert (streamtube); streamtube->render(); }

with:

void render_streamtube() const { if (!streamtube) return; streamtube->render(); }

Then, see if after some period of time the display starts working correctly.

LucSam commented 6 years ago

Dear Rob, You're welcome, I'm glad I can help. I just changed line 60 in edge.h which results in:

mrview: [SYSTEM FATAL CODE: SIGSEGV (11)] Segmentation fault: Invalid memory access

I can run it later on a debugger if that helps more.

Lestropie commented 6 years ago

Hmmm. I'd expected that bypassing that draw call would have been sufficient: I don't see anywhere else in the code where the streamtube data are accessed without verification. There might be something even more exotic going on, which unfortunately I have no hope of figuring out without some more debugger action... :crossed_fingers:

LucSam commented 6 years ago

@Lestropie Compiling MRtrix3 in debug mode failed because of the changed line 60:

debugger_fail_line60_changed.txt

Lestropie commented 6 years ago

It looks like you've retained the original line of code in addition to adding the new one, rather than replacing the line. There should only be one line of code that begins with "void render_streamtube()". It's possible that depending on your exact version of MRtrix3, the line number is not precisely accurate; the more important thing is to replace the relevant code.

LucSam commented 6 years ago

oops. So, after replacing the correct line..: There's no error showing up with lldb (and still no streamtubes).

This is the output of valgrind:

valgrind.txt

jdtournier commented 6 years ago

OK, I have to admit I'm not familiar with what's going on here - it's not my code... 😜

But looking through the code you point to, it strikes me that given this line, we should see a line with Generating connection streamtubes somewhere in the output - and I don't see it. So it looks like it's not even getting to Connectome::get_streamtubes(). Looking at where it's supposed to be invoked, it looks like this only happens if the variable Connectome::have_streamtubes is false. But a quick grep suggests that this variable is never initialised...

$ grep -rn have_streamtubes .
./src/gui/mrview/tool/connectome/connectome.cpp:1762:                if (!have_streamtubes) {
./src/gui/mrview/tool/connectome/connectome.cpp:3832:          have_streamtubes = true;
./src/gui/mrview/tool/connectome/connectome.h:330:            bool have_exemplars, have_streamtubes;

None of these occur anywhere near the constructor...

@LucSam, I think you need to add an explicit initialisation at line 85 in the file src/gui/mrview/tool/connectome/connectome.cpp, so it looks like this:

            ...
            have_exemplars (false),
            have_streamtubes (false),
            edge_fixed_colour { 0.5f, 0.5f, 0.5f },
            ...

That sounds like the most likely explanation to me...

Note that valgrind would have caught that, but it's poorly supported on macOS, and indeed didn't get very far at all in this case, bailing out when an unknown instruction was encountered... But given the code as it is, I'm guessing we should pick this up on Linux valgrind too.

Also of note: this is an AMD/ATI system, and their drivers are always more picky. You often get issues with the OpenGL handling not being exactly to standard, and it's rarely all that obvious when it isn't... But it doesn't look like this is the issue here (at least the outright segfault isn't...).

LucSam commented 6 years ago

@jdtournier thanks for the notes on valgrind and amd/ati systems.

Unfortunately, inserting have_streamtubes (false), didn't solve the problem..

jdtournier commented 6 years ago

Ok, when you say it didn't solve the problem, what exactly do you mean by that? Does it still crash out with a segmentation fault at the same point with the same assert failure? Or does it simply not render the streamtubes?

LucSam commented 6 years ago

Sorry for being unspecific. It doesn't render the streamtubes.

Edit: The crashing didn't occur anymore.

jdtournier commented 6 years ago

No need to apologise - it's hard to second-guess how our brains work...

So that's good news, in that at least that's one issue sorted. Can I check that this now runs without crashing or assert failures within the lldb session, using a -assert -debug config? If so, then we can start looking into the OpenGL handing as the prime suspect...

LucSam commented 6 years ago

Yes, I double checked: There is no crashing and no assert failure with lldbanymore.

jdtournier commented 6 years ago

Ok, thanks for confirming. We'll need to go over the OpenGL code and see if there's any issues like #1130...

Lestropie commented 6 years ago

Damnit, I was intentionally not providing a link to #1130; but now you've gone and dunnit... :sweat_smile:

If any devs on Mac OSX (esp. with ATI) could test out this code and see if they can reproduce, it would be very much appreciated.

jdtournier commented 6 years ago

Damnit, I was intentionally not providing a link to #1130; but now you've gone and dunnit... 😅

Had to be done...

On the plus side: this probably means the issue occurs on any ATI systems, macOS or not. We've got plenty of those in my department. Is there any test data for me to have a play with?

Lestropie commented 6 years ago

this probably means the issue occurs on any ATI systems, macOS or not.

Not sure; Was reported here on a GTX1060. But info is a little scattered due to the conflation with the node mesh visualisation issue.

Is there any test data for me to have a play with?

Test data from #1311 still applies.

jdtournier commented 6 years ago

Yes, hard to tell whether the issue on that NVIDIA system was purely locale-related. The user reports that things work, but might talking purely about the mesh issue. I guess we should ask them to clarify...

Lestropie commented 6 years ago

Reported on forum: Rendering edges as cylinders works, but streamtubes do not. It is a significantly different rendering path, but maybe there's a hint in there.