mrview Connectome tool surface visualisation issues

[Lestropie]

Two separate but perhaps related issues on the forum in short succession: Connectome node mesh visualisation problem Visualising edges as streamlines / streamtubes

What these two functionalities share is that they either import or internally generate triangular polygon data.

There may be some association of the issue with Mac OSX systems, particularly the streamtubes, but it's unclear at this point. If any dev can reproduce either of these faults please let me know, as otherwise I have no starting point.

[jdtournier]

Any chance you could share some data and instructions that ought to replicate the user's issues...?

Also, do you have access to a system with an ATI/AMD card? Their drivers tend to be a lot pickier than the NVIDIA ones, regardless of the OS - could explain at least some of these issues (assuming they are indeed running on ATI hardware)...

[Lestropie]

At least one of the users is on NVIDIA, so can't be purely an ATI thing.

Example data:

• mrview nodes.mif -connectome.init nodes.mif -connectome.load connectome.csv

• Problem 1: Nodes, Geometry -> Mesh, select nodes.obj file

• Problem 2: Edges, Geometry -> Streamtubes, select exemplars.tck file

[Lestropie]

Some updates on the node mesh visualisation issue on forum. Seems at least one source of problems is the mismatch in locale conformity between functions used to save (MRtrix3's str()) and load (sscanf()) floating-point data to/from the .obj text file; these should preferable be either a << / >> pair (i.e. str() and to()) or a C function pair (i.e. sprintf() / sscanf()). I haven't been able to find anything on the .obj format regarding how locale differences should be handled in that context specifically.

Within MRtrix3, both str() and to() are based on fresh instances of std::*stringstream, which will by default take the current C++ global locale. We're not making a std::locale::global(std::locale("")); call anywhere, which would set the C++ global locale to be the system default. In the absence of such a call, the C++ global locale is equivalent to std::locale::classic() (an immutable C++-compatible version of the C global locale), which is "the classic C locale semantics": the universal "simplest" locale. This is why our own converters "ignore the locale" as stated by @jdtournier on the forum.

(There's a secondary debate to be had here about whether or not stdin / stdout / stderr should be imbued with the system locale globally across MRtrix3)

This however doesn't explain why there would be issues in saving & loading .obj files entirely within MRtrix3: the C locale assumed by sscanf() and the C++ default locale assumed by str() should match... I could understand if the file were modified by some third-party software, and our own reader should ideally be as robust as possible against this, but it's not quite clicking for me why this is the source of the problem.

Nevertheless, if this specific instance is causing issues, it can be changed and tested. However while I initially had the idea of replacing & then forbidding all use of sscanf() and similar C functions via the check_syntax script, I then recalled having an issue with parsing of lookup table files using split() rather than sscanf(), specifically because I've seen at least one atlas lookup table in the wild that uses double-inverted-commas to encapsulate parcel names that contain spaces, which results in erroneous results from split(). So I'd need to improve upon #1281 by writing some kind of tokenize() function to complement split() before going full nuclear on scanf(); std::quoted would help, but that's C++14.

It's also worth noting that if this is indeed the source of the problem with node mesh visualisation, then the issue with streamtube visualisation would in fact not be related, contrary to the initial hypothesis of this issue.

[jdtournier]

Good bit of detective work on locale handling... I must admit I've never looked into it all that closely. But given the issues, it seems to be that it would be safer to set both the C and C++ locales to the default C early on in the init() call. That would require the fewest changes and lead to the most robust solution long term (since we can't guarantee what future devs might choose to use). What do you think?

[jdtournier]

OK, I had a look into this. It's a bit of a mess...

• turns out the C and C++ locale settings both default to C unless explicitly set. That's what they start as by default in any MRtrix3 app.

• Qt5 explicitly does something in the QApplication constructor: querying the locales before and after this line shows that the C locale is set to the system defaults at this point (i.e. according to the LC_* environment variables), but not the C++ locale.

• The QLocale::setDefault() call has no effect on either the C or C++ locales - irrespective of whether it is called before or after the QApplication constructor. Presumably it only affects Qt's own internal handling, which begs the question, why does it modify the C locale if it's not going to rely on it...? But then by Qt's own admission, their locale handling is shoddy...

• Setting the C locale via std::setlocale() leave the C++ locale unaffected, and setting the C++ locale via std::locale::global() leaves the C locale unaffected... So presumably we need to set both explicitly, after the QApplication constructor - any attempt at setting them before that call are overridden by that call...

Pull request to follow shortly...

[jdtournier]

See pull request in #1323. I'd set LC_ALL=hr_HR.UTF-8 for testing, which reproduces the issue. This seems to fix it for me...

[Lestropie]

Can't reproduce on Windows at home, so will need to have a look at work next week.

I think I agree forcing both c and c++ to classic locale semantics universally, for both GUI and non-GUI, is going to be the way to go. Thought about trying to set just cerr to use native encoding so that terminal feedback would be native, but anything that goes through str() rather than straight to operator<< would still not get that native encoding (at least unless e.g. str() took a bool argument to specify which encoding to use).

[jdtournier]

Checks out OK on my Windows 10 system. Compiles and runs fine, but then I couldn't reproduce the problem anyway... I think locale issues are handled differently on Windows... Anyway, I think we're good to merge.

Thought about trying to set just cerr to use native encoding so that terminal feedback would be native, but anything that goes through str() rather than straight to operator<< would still not get that native encoding (at least unless e.g. str() took a bool argument to specify which encoding to use).

Yes, this is really tricky to handle. We have to be able to distinguish direct user input/output from uses where the input/output is to files, where it's important to stick to the standard C locale to avoid funny issues when reading back the values on a different system (i.e. anywhere interoperability is possible). And it's not even that clear-cut, since terminal output can be redirected to file, which makes it a bit of a grey area. Personally, I think we have to force all locales to default to avoid any issues like this...

[Lestropie]

$ LC_ALL=hr_HR.UTF-8 mrview anat/vis.mif -connectome.init parc/nodes_fix_int.mif -connectome.load connectome.csv 

(process:6484): Gtk-WARNING **: 11:55:16.850: Locale not supported by C library.
    Using the fallback 'C' locale.

:man_facepalming: :rofl:

[Lestropie]

Okay, confirm this fixes the issue. Also confirm that the locale does not affect file write in any way (done here via str()), but a modified locale prevents correct file read via sscanf().

[Lestropie]

Closed by #1323; re-listing streamtube visualisation issue in #1330 as that will not be fixed by the localisation handling changes in #1323.

[jdtournier]

(process:6484): Gtk-WARNING **: 11:55:16.850: Locale not supported by C library.

Yes, this depends on how your system is set up. On my Arch Linux box, only the locales I've set up are available. I had to enable that particular one for testing... I guess the locales installed by default, and the process for generating additional ones, will be distro-dependent.