add manifold check

[akaszynski]

Implement suggestion from @adamgranthendry in pyvista/pyvista-support#510 for a manifold check.

[codecov]

Codecov Report

Merging #1684 (5143891) into main (0bf59f6) will increase coverage by 0.00%.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##             main    #1684   +/-   ##
=======================================
  Coverage   30.84%   30.84%           
=======================================
  Files          74       74           
  Lines       15395    15398    +3     
=======================================
+ Hits         4748     4750    +2     
- Misses      10647    10648    +1     

[adeak]

I've been doing mesh.n_open_edges == 0. We use that internally too in a few places. How do these relate?

[akaszynski]

I've been doing mesh.n_open_edges == 0. We use that internally too in a few places. How do these relate?

Turns out that they're identical. I missed n_open_edges and I think that it's not obvious enough to warrant a separate method for returning if the mesh is manifold.

[adamgranthendry]

I've been doing mesh.n_open_edges == 0. We use that internally too in a few places. How do these relate?

Semantically, I wouldn't know to use "open edges". The docs don't explain what is is meant by an "open edge" or provide any visuals. When most (if not all) VTK algorithms fail, they do so with an error that the mesh "is not manifold" rather than it having "open edges", or something else. I think it makes more sense to the end user to use the term "manifold" in the property (that's what they're going to be looking for; something that tells them if the mesh is manifold or not).

There are also multiple reasons why a mesh might not be manifold (self-intersecting faces, open edges, duplicate vertices, edges, or faces, to name a few). So, it would be good to know not only whether the surface is manifold, but also why it is not manifold, and where the guilty vertices, edges, and faces are.

To that end, I think it would be good to have an is_manifold property (maybe it can be the "logical AND" of several sub-properties), in addition to other "sub-properties" like self_intersecting_faces, duplicate_vertices, duplicate_edges, etc. Improved documentation with full definitions and examples would go a long way here as well (re-using the name "open edges" in the definition doesn't really clarify things).

[adamgranthendry]

Implement suggestion from @adamgranthendry in pyvista/pyvista-support#510 for a manifold check.

Thank you @akaszynski ! Sorry for dropping the ball on this...

[darikg]

strongly recommend a more intuitive spelling of this such as is_watertight. pyvista.Plane is a bounded manifold!

edit: how about has_boundary?

[adamgranthendry]

strongly recommend a more intuitive spelling of this such as is_watertight. pyvista.Plane is a bounded manifold!

I like this thinking as well. There are several definitions used interchangeably that people in different fields might be more used to. The term "manifold" has a comprehensive mathematical definition, but I have also heard and see the term "water tight" being used.

I think all of this could be fleshed out a bit more in the docs.

I am still a fan of adding a few "manifoldness/watertightness" properties (i.e. "duplicate vertices", "duplicate edges", "duplicate faces", "self-intersecting faces", "open edges", etc.) in addition to a larger property "is_manifold/is_watertight" that captures all of them.

I'm personally a fan of using "is_manifold" because VTK and pymeshfix use the term in its errors, it is a much more comprehensive term (applies to any n-dimensional object). We could use the term "water tight" to help describe what it means in the docs though.

[akaszynski]

pyvista.Plane is a bounded manifold

Good point. I was going for "compact manifold" (e.g. a pyvista.Sphere).

To that end, I think it would be good to have an is_manifold property (maybe it can be the "logical AND" of several sub-properties), in addition to other "sub-properties" like self_intersecting_faces, duplicate_vertices, duplicate_edges, etc. Improved documentation with full definitions and examples would go a long way here as well (re-using the name "open edges" in the definition doesn't really clarify things).

These checks will involve a bit more logic but it's doable. I know that certain libraries (like tetgen) require a compact manifold. Furthermore, is a compact manifold mesh with duplicate vertices or edges still a compact manifold? Watertight might be a better descriptor, but nonetheless, I'd consider a watertight mesh still a compact manifold.

Can we come up with a better name than watertight? If not, I'll roll with it and introduce additional validation checks.

[adamgranthendry]

Can we come up with a better name than watertight? If not, I'll roll with it and introduce additional validation checks.

I'm also more of a fan of using manifold in the term. We should probably also consider using the term "water tight" in the definition somewhere as well though. Some people might understand water tight, but not manifold, or vice-versa.

As mentioned, I don't think any errors a user will see say anything like "edges were open" or "mesh was not watertight", but rather "mesh is not manifold", so I think it will make more logical sense across multiple libraries if we use the term is_manifold and just provide a really good definition of what it means with some illustrations and examples :).

[darikg]

Two suggestions:

1. Rename n_open_edges to n_boundary_edges
2. Define has_boundary to return self.n_boundary_edges > 0

Because only looking at the number of open/boundary edges cannot determine if the object is manifold or watertight or any other higher-order property really

[darikg]

As mentioned, I don't think any errors a user will see say anything like "edges were open" or "mesh was not watertight", but rather "mesh is not manifold", so I think it will make more logical sense across multiple libraries if we use the term is_manifold and just provide a really good definition of what it means with some illustrations and examples :).

@adamgranthendry do you remember specifically which VTK errors you were thinking of here? I would rather fix them (either upstream or as a wrapper in pyvista if really necessary) rather than propagate an incorrect meaning of 'manifold'.

[adamgranthendry]

As mentioned, I don't think any errors a user will see say anything like "edges were open" or "mesh was not watertight", but rather "mesh is not manifold", so I think it will make more logical sense across multiple libraries if we use the term is_manifold and just provide a really good definition of what it means with some illustrations and examples :).

@adamgranthendry do you remember specifically which VTK errors you were thinking of here? I would rather fix them (either upstream or as a wrapper in pyvista if really necessary) rather than propagate an incorrect meaning of 'manifold'.

I'm not referring to specific errors, but in general. For example, if you smooth a mesh such that it becomes non-manifold and then attempt to decimate or subdivide it, I have seen an error like:

VTK Error: mesh is not manifold

My point is that manifold is a more common term than water tight. "Water tight" is also not a technical term, whereas manifold has a precise mathematical definition. As a result, I think it is the better term to use because I think more people will be familiar with it over water tight and are more likely to see it as the term pops up in errors (whereas "water tight" does not).

There is nothing to fix. These sorts of things can be prevented by checking before successive filters whether or not the mesh is manifold, but hence that's why this PR has been created.

Currently, one could check for n_open_edges or make an is_manifold function like I presented in pyvista/pyvista-support#510.

Since this would be a common thing to check for, it is appropriate to add it to the list of properties existing for data sets and manifold is the proper term to use that most will be familiar with and come across.

[akaszynski]

I'm of the opinion that we should keep the "terminology" of PyVista in concert with VTK, and therefore should use the same general definition of "manifold." Moving to merge this unless there are any significant objections.