Extrude and Resolve should be static on Manifold.

[briansturgill]

OK, the python binding has the Extrude and Revolve API calls as being member functions on CrossSection, when
in fact they are static on Manifold.
This PR fixes the problem.
#691

[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.

[elalish]

@wrongbad would you like to take a look at this PR? Technically this is a breaking change, and it's a touch arbitrary. We leave this as a static method in the C++ to keep CrossSection from having a dependency on Manifold, but they're all combined in the Python lib anyway. Still, consistency does seem nice from a docs perspective.

@briansturgill I believe the CLA is an automatic click through if you sign as an individual. It's pretty standard stuff, though slightly annoying.

[briansturgill]

@elalish CLA has now been signed.
Sorry, I didn't realize you had automated testing for Python and didn't fix the test.
Can I amend a pull request?
The issue isn't entirely arbitrary. It's kind of an accident that it was working before.
Because the first argument was a CrossSection and "this" is passed in most C++ implementations
as a first argument. It accidently worked as a member function... but it seems unwise to leave it that way.

[elalish]

Ah, good call, thanks for the explanation. Yes, just add another commit to your branch and push - the tests will automatically rerun.

[briansturgill]

I have no idea what to do about the code formatting. It is very unclear from the "detail" what is being complained about.

[elalish]

Have you tried our formatting script? https://github.com/elalish/manifold?tab=readme-ov-file#formatting

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (887a502) 91.67% compared to head (9822541) 91.67%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #692   +/-   ##
=======================================
  Coverage   91.67%   91.67%           
=======================================
  Files          37       37           
  Lines        4730     4730           
=======================================
  Hits         4336     4336           
  Misses        394      394           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[wrongbad]

From my POV...
pro: consistency with c++, and c++ documentation
con: less chainable (e.g. CrossSection(pts).offset(2).mirror((0,1)).extrude(10)) but that can be worked around.
I'm fine with it.

[geoffder]

You pass in a function to use for the binding, which just happens to be a static method on Manifold, I wouldn't say that is "on accident".

Also I don't really see the point of simultaneously removing it from CrossSection since it is convenient to have there. We only don't have it that way in CPP because of the dependency.

[briansturgill]

@geoffder
The file instructed nano bind that a static method was a member method.
Nanobind caused the call to be cs.member(...)
However, that was really a call to another class which was static, fortunately it needed cs as it's first argument.
It accidentally worked because clang does member calls by pushing "this" on the stack as the first argument.
There is nothing in the C++ standard which says that must work.
Different compiler, potentially a different result.
Regardless, it is invalid code and probably shouldn't be used.

[geoffder]

Could you point me to documentation about this nanobind method making these assumptions (that it is a method, rather than a static one) about the provided function? As far as I understand, you can pass any function into .def (including plain, non-member functions).

For example, here we use a lambda:

manifold/bindings/python/manifold3d.cpp

Lines 268 to 273 in 887a502

	.def(
	"rotate",
	[](const Manifold &self, glm::vec3 v) {
	return self.Rotate(v.x, v.y, v.z);
	},
	nb::arg("v"), manifold__rotate__v.c_str())

In any case, if this usage really is in error, then the solution should rather be to write a wrapper function, than to remove the functionality entirely.

[wrongbad]

The file instructed nano bind that a static method was a member method.
Nanobind caused the call to be cs.member(...)
However, that was really a call to another class which was static, fortunately it needed cs as it's first argument.

Nanobind is a strongly typed c++ library. The only way to do what you're saying is to cast the function pointer through a void* or reinterpret_cast. You're right that instance-first-argument is not part of the spec, but that's probably also why c++ doesn't allow implicit conversions between member function pointers and non-member function pointers.

Nanobind has a template/overload which picks up non-member functions (e.g. all the lambda wrappers are non-member functions) accepting the object as the first argument. There's nothing unsafe or undefined about the code.

It's honestly pretty hard to read, but you can see the function wrapper overloads here: https://github.com/wjakob/nanobind/blob/master/include/nanobind/nb_func.h

Edit: here's the part that matters:

template <typename Return, typename Class, typename... Args, typename... Extra>
NB_INLINE object cpp_function(Return (Class::*f)(Args...), const Extra &...extra) {
    return steal(detail::func_create<true, true>(
        [f](Class *c, Args... args) NB_INLINE_LAMBDA -> Return {
            return (c->*f)((detail::forward_t<Args>) args...);
        },
        (Return(*)(Class *, Args...)) nullptr,
        std::make_index_sequence<sizeof...(Args) + 1>(), extra...));
}

basically whenever you pass an actual member function, nanobind internally wraps it in a lambda to become a function accepting the instance as the first argument, so it stores a consistent data structure for python to call later. When you pass a non-member function, this lambda step is skipped (using a different overload) because it's already the right function signature.

When in doubt, read the code ;)

[pca006132]

I don't think it is an issue, and it is probably not very useful to make it a static function on manifold for the python binding considering it is much easier to use it as method chaining.

[pca006132]

Any consensus on this? I don't think this PR is needed.

[elalish]

Perhaps just add the static method to the Python as well? Then it's not breaking, but also matches our C++ better.

[pca006132]

sounds good
@briansturgill do you want to do that in this PR?

[briansturgill]

@pca006132
Yes, I'll do it today. I'll get compose added as well.

[elalish]

Looks great, thanks!

[elalish]

Maybe leave one example using circle.extrude() for variety? Unless there's already one that you didn't change.

[pca006132]

I think if we are having it in the all_apis.py it should be fine.