Closed bjeurissen closed 1 year ago
From the above stack trace, the culprit appears to be (size_t) upsample_ratio = 18446744073709551615
, which corresponds to the largest uint64. This leads to M.resize (18446744073709551614, 4)
. Clearly, something is going wrong.
Found the culprit: https://github.com/MRtrix3/mrtrix3/blob/f633dfd7e9080f71877ea6a4619dabdde99a0fb6/cmd/tckmap.cpp#L275
For some reason this does not work as expected in combination with recent xcode... Instead of a vector of ones, this results in a vector of extremely small values.
OK, was looking into it, and really can't replicate any issues on Linux...
Can you try using:
voxel_size.resize (3, voxel_size.front());
instead of that line?
indeed, I just tested resize and that works! Will push a fix
Hang on, I'm not sure it's a safe fix!
Too late ;)
I think the issue is that std::vector::front()
(and std::vector::operator[]
) return a reference to the first element, not a copy. The first thing that the assign()
(or resize()
!) call might do is a memory re-allocation, which presumably involves deleting the original memory location, and so invalidate the data referenced in the earlier .front()
call. The fact that it works is not an indication that it's safe, I think this is still technically undefined behaviour, whether we use .assign()
or .resize()
.
The proper fix here would be something like:
274 if (voxel_size.size() == 1) {
275 auto v = voxel_size.front();
276 voxel_size.assign (3, v);
277 }
... or potentially:
voxel_size.assign (3, default_type (voxel_size.front()));
since that will hopefully force a non-reference temporary value as the second argument...
Thanks, I pushed the long option just to be sure it will work as expected
CI for tckmap causes a failed assertion on macOS(other platforms appear to be fine):
As raised in #2472 and #2566
More detail:
Is this because macos CI uses a different eigen version than linux and windows?--> same problem with eigen 3.3 Or is this due to an update of macOS/xcode?