Skip to content

clip closed surface for PolyData#797

Merged
banesullivan merged 2 commits intomasterfrom
feat/clip-closed-surface
Jun 17, 2020
Merged

clip closed surface for PolyData#797
banesullivan merged 2 commits intomasterfrom
feat/clip-closed-surface

Conversation

@banesullivan
Copy link
Copy Markdown
Member

@banesullivan banesullivan commented Jun 13, 2020 •
edited
Loading

See #794

This only supports single plane clipping though the underlying VTK filter supports plane collections.

import pyvista as pv
mesh = pv.Sphere()
cclipped = mesh.clip_closed_surface(normal='-x')
oclipped = mesh.clip(normal='-x', invert=False)

p = pv.Plotter(shape=(1,2))
p.add_mesh(cclipped, show_edges=True)
p.subplot(0,1)
p.add_mesh(oclipped, show_edges=True)
p.link_views()
p.view_isometric()
p.show()

download

@banesullivan banesullivan mentioned this pull request Jun 13, 2020
Closed
@banesullivan banesullivan added the enhancement Changes that enhance the library label Jun 13, 2020
@akaszynski
Copy link
Copy Markdown
Member

akaszynski commented Jun 14, 2020

Waiting to review this to see if @yetisir wants to take a look at it.

@akaszynski akaszynski mentioned this pull request Jun 14, 2020
Open
@banesullivan
Copy link
Copy Markdown
Member Author

banesullivan commented Jun 14, 2020 •
edited
Loading

I’m not sure what would be the most pythonic way to support plane collections as the underlying filter typically takes more than one plane.

Should we implement origin and normal being lists of the same length? I feel the type checking there could get messy

@yetisir
Copy link
Copy Markdown
Contributor

yetisir commented Jun 14, 2020

Awesome! Thanks @banesullivan - that is exactly what I had in mind.

Wrt multiple planes - I agree things could get messy. My previous implementation only accepted one plane, and when I needed to use multiple planes (rarely) I would just pass the mesh through the filter for each plane.

For simplicity - I think just one plane is perfectly fine - that way the interface is the same as the clip() method. I think most users would only be looking to clip with one plane anyways.

LGTM

@akaszynski
Copy link
Copy Markdown
Member

akaszynski commented Jun 14, 2020

Wrt multiple planes - I agree things could get messy. My previous implementation only accepted one plane, and when I needed to use multiple planes (rarely) I would just pass the mesh through the filter for each plane.

I think this sounds like the simplest and most intuitive approach.

akaszynski
akaszynski approved these changes Jun 16, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

Checking for it to be manifold was a good touch. Thanks for adding this!

@banesullivan banesullivan merged commit f4839f3 into master Jun 17, 2020
@banesullivan banesullivan deleted the feat/clip-closed-surface branch July 21, 2020 20:06
@akaszynski akaszynski mentioned this pull request Sep 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akaszynski akaszynski akaszynski approved these changes

Assignees

No one assigned

Labels

enhancement Changes that enhance the library

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@banesullivan @akaszynski @yetisir