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

Modify vmtksurfaceendclipper to clip based on centerline information #200

Merged
merged 3 commits into from Jan 10, 2018

Conversation

Projects
None yet
2 participants
@kayarre
Contributor

kayarre commented Oct 18, 2017

per comments from @rlizzo in #196 I added clipping using centerline information feature to the vmtksurfaceendclipper.

This modification provides a means to use the normal information from the frenet frame of reference along the centerline to clip at inlet and outlet locations. The seed selection requires that the users knows the inlet and outlet direction used in the centerline generation such that the proper normal direction is applied in the clipping.

The issue with the current implementation of surfaceendclipper is two things.

  1. The normal estimation is poor
  2. The normal may be inverted and clip the whole surface. The vtkClipPolyData section could be updated to catch the instance when the normal is inverted and select the proper clipped body, however It seemed easier to use the centerline information.

The additions also includes removing the use of python dictionaries in the for loop.

@rlizzo

This comment has been minimized.

Member

rlizzo commented Jan 10, 2018

Thanks for this PR @kayarre! I'm really sorry it took so long to review this! It works really well. Nice Job!

Also, I meant to just push a few small changes to your PR before merging it (just updated the docstring and simplified some expressions), but it appears that I have royally screwed up and somehow merged in every commit to master from the point you branched to now... Not entirely sure how that happened. Sorry about the confusion, will fix now.

kayarre and others added some commits Oct 18, 2017

@rlizzo

This comment has been minimized.

Member

rlizzo commented Jan 10, 2018

Ok. history crisis averted by rebasing on master. Merging now!

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