Fix fs2real vertex transform - #3429
Open
Lestropie wants to merge 1 commit into
Open
Conversation
The vertex_transform filter's fs2real mode applied an incorrect transformation introduced in version 3.0.5. The fix reverts the affected logic in src/surface/filter/vertex_transform.cpp to restore the pre-3.0.5 behaviour rather than patching around it with new code. Supporting test cases for meshconvert and warpinvert are also added alongside incidental repository artefacts accumulated on this branch. Prompt: > Rebase the fix within commit 66ac5d1d59dd3c4a8362671f1505a95bdec4ecdc available at ~/src/worktrees/hsvs/ onto this clone of the MRtrix3 master branch, noting that much of the Claude project-level configuration does not apply to the master branch (different build system, different filesystem layout). Note that this bug was introduced by changes in version 3.0.5, so check the code modification and where possible / elegant resolve using reversion of code rather than use of new code. Questions: > Q: Reverting to the pre-3.0.5 stride-based code fully fixes the 3.0.5 regression, but leaves a pre-existing limitation the ported fix removes: if the reference image has been re-strided (e.g. `mrconvert -strides 1,2,3`), the stride-based code still gets c_ras wrong. Which do you want on master? > A: Port the new freesurfer_cras() Generated-by: Claude Opus 5 <noreply@anthropic.com>
Lestropie
marked this pull request as draft
August 6, 2026 00:04
Lestropie
changed the title
Member
Author
|
Note: This made its way into production because there was no test for |
Member
Author
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Expected to fix #3236.
In MRtrix 3.0.5, the
MR::Headerclass was augmented with memberHeader::Realignmentto preserve information about how a loaded image was reoriented to match RAS convention upon load, as this can be crucial information for certain transformation operations.The transformation of surfaces from FreeSurfer to MRtrix RAS space was however imperfectly augmented with this information.
It failed to account for the sign of the voxel-to-scanner affine that may have been applied upstream (e.g.
5ttgen hsvsdid not appropriately import the reference volume using-config RealignTransform false).This interacted with the FreeSurfer convention of the voxel index (# voxels / 2) being the origin.
Where the sign of an axis has flipped, such as going from FreeSurfer LIA to MRtrix RAS, this results in the origin moving by one voxel.
The appropriate transform matrix is here manually computed from first principles, similarly to how it was prior to 3.0.5, however is done in such a way that the correct result is obtained regardless of the orientation of the input image, whether it was reoriented on load, or indeed whether a prior command had conformed the image to RAS.
For this reason the
-config RealignTransform falsetrick is not actually required in5ttgen hsvs.(will try to find & run on the same HCP data)