Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to enable meshconvert to warp mesh files. #2617

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

Arshiyasan
Copy link

As suggested by @Lestropie, this enhancement enables users to specify a new mode for meshconvert through setting -transform warp <warp image> to apply a deformation field to mesh files. The approach is similar to tcktransform. It works well on my data!

@@ -81,6 +85,10 @@ void run ()
case 2: transform->set_voxel2real(); break;
case 3: transform->set_real2voxel(); break;
case 4: transform->set_fs2real (); break;
case 5:
transform->set_warp();
transform->set_warp_image(Image<float>::open(opt[0][1]));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
transform->set_warp_image(Image<float>::open(opt[0][1]));
transform->set_warp_image(H.get_image<float>());

Also transform->set_warp() shouldn't be necessary; it should be possible to set that internal flag when set_warp_image() is called.

@Lestropie
Copy link
Member

  • I think I would prefer to have an explicit meshtransform command.
    Once one starts providing non-linear deformation fields, it's no longer just "a conversion between mesh formats".
    So just as for mrtransform and tcktransform, meshtransform could use linear or non-linear transforms.
    meshconvert would then likely be exceptionally simple, since the linear transformation options would be moved to meshtransform, but it should nevertheless remain there for discoverability; and any more advanced controls regarding export (eg. switching between VTK binary and ASCII) would belong there.

  • Would be nice to have meshtransform accept a "full" warp file, and control the target and destination spaces of the transformation. However this is not strictly requisite for the functionality to be added.

  • Wherever more than a sentence is required to explain the operation of a command-line option, I prefer to move the primary description out of the option's help string and into the command description / example usages. Eg. Particularly if an option to support full warps is not implemented, an example usage where the output of warpconvert is piped to meshtransform could be specified and described verbosely.

  • Currently, if a vertex lies outside of the warp image FoV, an uninitialised vertex is pushed. This will corrupt any transformed mesh. Given that we are exclusively dealing with closed surfaces, I think the only solution is to yield an error if no valid warp information can be accessed for just one vertex.

  • The vertex sanity check currently only checks the warp image FoV. There are however other instances where no valid warp information is present: NaN-filling (easy to detect), and zero-filling (harder to detect, but there should be a wrapper class somewhere that detects such).

  • Not strictly related to the proposed change, but it comes to mind looking at the code:
    Here function File::NIfTI::adjust_transform() is being used in the "fs2real" function. However as occurs elsewhere (most recently mrconvert: fslgrad and RealignTransform: 0 #2477), this may instead actually be dependent on the transform realignment that occurs during image load, not what would hypothetically occur on NIfTI write.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants