fix for #2269 with new test case for incomplete creation.revolve()

[bguan]

Added a cap: bool parameter to creation.revolve(...) and implement additional logic to add end cap faces to an incomplete revolution so that the resultant mesh is a valid volume.

[mikedh]

Awesome thanks for the PR looks good!

Yeah with the cap arguments the repairs might be needed because triangulate_polygon can sometimes depending on arguments insert vertices, which totally messes up the index. We've seen that in slice_mesh and sweep too, maybe we should add a triangulate_polygon(..., force_same_vertices: bool = False) argument which modifies the arguments and raises if the triangulation engine inserted anything.

I think we should also probably be raising in tol.strict mode here just to make sure in simple cases we've indexed correctly:

    # HACK: minor repairs if needed
    if not closed and cap and not mesh.is_volume:
      mesh.update_faces(mesh.nondegenerate_faces())
      mesh.update_faces(mesh.unique_faces())
      mesh.remove_infinite_values()
      mesh.remove_unreferenced_vertices()
      mesh.fix_normals()

[bguan]

Thanks @mikedh. Sorry for not checking the formatting and leaving a bunch of unnecessary white spaces around triggering those test failures. I will amend the next PR to take out those pesky trailing whitespaces.

As you've confirmed my fear that triangulate_polygon(...) can sometimes add new vertices, I can change my capping logic to just use and add the vertices returned by it instead of reusing existing vertices, use trigonometry to transform them to find the end cap vertices and also add them, use cap_0_faces indices to create cap_end_faces... this will ensure correct behavior even if triangulate_polygon(...) introduces new vertices. BTW is there a mesh repair to consolidate duplicate or close enough vertices that takes care of changing face indices? As for your idea of adding a force_same_vertices param to triangulate_polygon(...) I will leave you or others to implement it 😄

Also to clarify about your comment on my HACK checks... did you mean I should amend my repair conditions to be like this:

    #  vvvvvvvvvv
    if tol.strict and not closed and cap and not mesh.is_volume:
        ...

I can certainly do that in an updated PR.

Also wondering if you think my repairs are excessive. For example:

      ...
      mesh.remove_infinite_values()
      mesh.remove_unreferenced_vertices()
      ...

Thanks again, and all suggestions welcome.

[mikedh]

As you've confirmed my fear that triangulate_polygon(...) can sometimes add new vertices, I can change my capping logic to just use and add the vertices returned by it instead of reusing existing vertices, use trigonometry to transform them to find the end cap vertices and also add them, use cap_0_faces indices to create cap_end_faces... this will ensure correct behavior****

Yeah one of the challenges of supporting multiple triangulation backends is sometimes they insert vertices. This has come up elsewhere too, I'll add the force_same_vertices boolean to triangulate_polygon and raise a ValueError if it inserts so from the point of view of this function we can just rely on it not inserting.

Also wondering if you think my repairs are excessive.

Heh yeah it probably shouldn't need the finite/unreferenced check. In general I think we probably shouldn't need to run any repairs if we've done the indexing carefully and there have been no inserted vertices.

We can also probably reconstruct the indexes from a subset with a radius, index, = scipy.spatial.cKDTree(mesh_vertices).query(new_vertices) and then assert (radius < 1e-12).all().

[mikedh]

Thanks for the PR!! I'll add the force_same_vertices: bool to triangulate_polygon so we don't have to handle that in this cap logic.