python bindings: mirror and Mesh with 2 args

[axel-angel]

Hello there,

I have noticed those two things are missing in the python bindings. I checked other methods and didn't see anything obvious more to add so far, it's a small PR.

• The mirror() function seems to be not exposed to Python.
• The Mesh() constructor in C++ has 2 optional arguments but the Python bindings force it (with those .unchecked it just refused the call). I'm sorry I did a copy paste, that looked like the easiest and simplest way to achieve it. Let me know if you have an idea how to make this better.

I tested with my Python code. Hope this helps!

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (6ae9dee) 89.94% compared to head (b39ff1f) 89.94%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #459   +/-   ##
=======================================
  Coverage   89.94%   89.94%           
=======================================
  Files          35       35           
  Lines        4286     4286           
=======================================
  Hits         3855     3855           
  Misses        431      431           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[elalish]

@pca006132 It looks like the existing Mesh constructor is intended to have option arguments, but @axel-angel says they're not working that way. I'm not the best with Python, but I assume there must be some canonical way to do optional arguments so extra functions are not necessary?

[elalish]

Have you managed to sign the CLA yet? The formatting should be taken care of if you run clang-format.

[axel-angel]

Have you managed to sign the CLA yet?

Yes I did sign.

The formatting should be taken care of if you run clang-format

I copy pasted the code from above, so the format is the same (unless I mixed tabs vs spaces)?

but I assume there must be some canonical way to do optional arguments

As said above, it comes from those "unchecked" calls. Maybe we can check for null and only do unchecked for those with the rest of the processing? I could check if it works on my side. Let me know if you have thoughts.

ValueError: array has incorrect number of dimensions: 1; expected 2

[pca006132]

The canonical way for optional argument is

manifold/bindings/python/pymanifold.cpp

Line 591 in 6ae9dee

py::arg("fillrule") = CrossSection::FillRule::Positive,

It should be simple to fix.

[axel-angel]

Thanks to a post in pybind/pybind11#1953 (comment) I was able to properly make the argument optional. I also formatted using clang-format. 👌

[pca006132]

I think we can just have empty array as the optional argument. That can simplify the code a bit.

[axel-angel]

I couldn't find how to create empty arrays of specific dimensions. Any hint?

          py::arg("vert_normal") = py::array_t<float>(shape={0, 3}),
          py::arg("halfedge_tangent") = py::array_t<float>(shape={0, 3}))

this fails

manifold/bindings/python/pymanifold.cpp:516:55: error: ‘shape’ was not declared in this scope; did you mean ‘unshare’?
  516 |           py::arg("vert_normal") = py::array_t<float>(shape={0, 3}),
      |                                                       ^~~~~
      |                                                       unshare

[pca006132]

           py::arg("vert_normal") = std::vector<glm::vec3>(),
           py::arg("halfedge_tangent") = std::vector<glm::vec4>())

[axel-angel]

@pca006132 Unfortunately it fails at auto vertNormal_view = vertNormal.unchecked<2>(); line in the original code:

ValueError: array has incorrect number of dimensions: 1; expected 2

I wish I had a clue how to fix this.

[pca006132]

try std::vector<std::vector<float>>? not on my computer now so cannot test this

[axel-angel]

@pca006132 Thanks. Still the same. I think it's expecting a real (allocated?) array because in python I was able to provide an empty as follows:

        m = pymanifold.Mesh(vertices.astype(np.float32),
                            faces.astype(np.int32),
                            # FIXME: Manifold forces Python binding to send arrays here
                            np.empty(shape=(0,0)), np.empty(shape=(0,0))
                            )

See np.empty. No idea how to reproduce that in C++

[pca006132]

No idea, a simple fix would be to provide a basic python script wrapper, similar to what we do with js.

[axel-angel]

Any update? I'm proposing the following: (1) merging as is, (2) separating the change for mirror() into new PR so we can merge it and keep the Mesh() only here, so we can think about it (3) anyone can make that wrapper? I have no idea how to do it

[pca006132]

yeah I think we can just merge this