VTK surface format changes

[Lestropie]

• Change binary export to single-precision floating-point.
• Change binary export to big-endian.
• Support import of both little-endian and big-endian data.

Closes #2593.

Given the comments in the code I suspect this was pushed as a "well this works for its current purpose" but wasn't as robust as it should have been.

Changes result in successful loading in ParaView for both binary data generated by MRtrix3, and those same data loaded into MRtrix3 and then exported as ASCII (so MRtrix3 is reading from those data correctly).

Remaining question:

• Whether test data needs to be added.
• Whether master is the right destination: it "fixes a problem", but it also changes the output to single rather than double precision, which could be construed as a "behaviour change". Son't think this is an issue, just listing anyways.

[jdtournier]

OK, I've had a quick search around the topic, and it does indeed look like legacy binary VTK format is explicitly big-endian. This is stated unambiguously on the VTK wiki. Oddly (at first), it states that this is explicitly mentioned in main VTK docs from which your quote originates, and to me, that quote strongly suggests it should support both encodings.

Reading that quote again (as far as I can tell, this is the only place in the docs where this is discussed), it may just be worded spectacularly badly, but in that case, it's really bad. With that it mind, when it says it 'manages the byte ordering for you', I guess it means it'll swap endianness for both read & write on a little-endian system (not that it'll swap endian-ness at read-time as required, which is how I'd initially interpreted that) . When it states "For example, binary files written on a Sun are stored in big endian order, while those on a PC are stored in little endian order", it's probably talking about the generic action of writing binary data on these systems (not what the VTK IO routines do, which is how I'd initially interpreted that). The next line is really ambiguous: "As a result, files written on a Sun workstation require byte swapping when read on a PC" - yes, since they're stored big-endian - but for me, it implies the converse might be true: that files written on a PC might require byte swapping when read on a Sun workstation - but it doesn't actually say it. The final line is also super-ambiguous: "The VTK data files described here are written in big endian form" really suggests that little-endian would also be considered valid...

Regardless, I reckon we should go off the much clearer VTK wiki statement, and definitely write all outputs big-endian - as you've done here. I'm in two minds about what to do with the existing little-endian format, given that it sounds like it is technically invalid. If you have a robust way of detecting when that happens (at least for the files produced with previous versions), then you're right that we should maintain some form of backward compatibility with these data. The only thing I would personally add is to issue a warning when such data are encountered with instructions for converting the data back to big-endian (presumably using meshconvert?). What do you reckon?

I note there is also a newer XML-based standard where endianness is stated explicitly. Not sure whether we want to look into implementing that and deprecating the legacy VTK binary format, but if we do, that's a story for another day...

[jdtournier]

Remaining question:

• Whether test data needs to be added.
• Whether master is the right destination: it "fixes a problem", but it also changes the output to single rather than double precision, which could be construed as a "behaviour change". Son't think this is an issue, just listing anyways.

I think it would be good to have tests for binary data, with a binary diff against a known-good output. Only works if the output is fully deterministic (no timestamps, software versions, etc).

And yes, I think master is the right place for this to go - binary outputs are currently unusable in 3rd party applications, that clearly sets it out as a bug fix, not a feature.

[Lestropie]

Commented on #2593 before reading here; looks like @jdtournier's reading of the text is the same as my own.

The only thing I would personally add is to issue a warning when such data are encountered with instructions for converting the data back to big-endian (presumably using meshconvert?).

Yep, I did essentially that after contemplating @neurolabusc's comment; see 11d3bf2.

[Lestropie]

Included in the test data here is a binary VTK generated using the current master code (which is erroneously little-endian) and another generated using the code in this branch (which is big-endian), alongside the existing ASCII version of the same data. ParaView crashes on attempting to load the little-endian version. MRtrix3 reads all three, but issues a warning on reading the little-endian version. So it should be good to merge unless anyone can spot something in the code or think of another requisite test.