trelau / pyOCCT_binder

Binding generator for pyOCCT project
GNU Lesser General Public License v2.1
7 stars 1 forks source link

pySMESH support #21

Open frmdstryr opened 3 years ago

frmdstryr commented 3 years ago

This adds changes to support for generating bindings for pySMESH. The pyOCCT binder/run.py will need updated as this changes the Generator parameters.

Summary of changes:

Also new PR for pySMESH incoming...

trelau commented 3 years ago

Thanks for the PR! Would it be possible to add some simple tests to capture these changes? I'm trying to build up the test cases here to avoid not learning something breaks until it happens in downstream processes in pyOCCT or pySMESH.

Also, are you expecting to target SMESH 9.X? My general plan has been:

  1. Get pyOCCT up to latest version of OCCT (done)
  2. Get SMESH up to latest version of Salome and (done?)
  3. Get pySMESH synced up and with above and the binding generator (in progress, it seems)
trelau commented 3 years ago

In general, it might be useful when we find something that is not working or not being wrapped properly downstream, we manually find the correct binding, then bring that (or something that replicates it) to this project as a test, given the expected output, and then work on the binder until we can pass all tests.

trelau commented 3 years ago

@frmdstryr do you think we should get working branches here and in pyOCCT and pySMESH before merging this? I'm ok merging it, you just mentioned another PR so I was checking to see if you preferred to get everything queued up and merged all at once.

frmdstryr commented 3 years ago

In general, it might be useful when we find something that is not working or not being wrapped properly downstream

Agree, I'll take the parameter signature change out and see If I can reproduce it in a test.

do you think we should get working branches here and in pyOCCT and pySMESH before merging this?

It doesn't hurt to wait, the submodule can be changed easily later.

trelau commented 3 years ago

What's up with the windows failure? Looks like it started with the "...compile test..." commit. What's that for?

frmdstryr commented 3 years ago

The thought was to test the reason for keeping the std:: parameters which only showed up at compile time. I could just remove that test.

Perhaps keeping the parameter signature should be some sort of binder option?

trelau commented 3 years ago

so you're saying that std:: gets removed in the generated bindings? in most cases i would think we just need to capture the text of the parameter and copy it into the bindings.

tbh i think clang makes this more complex than it should be, but that's an issue for another time i suppose. i while back i tried using this project to preprocess the headers and write a binding generator without clang. it actually seemed to work mostly well but i uncovered a handful of edge cases (see the issues tab) and development has seemed to stop https://github.com/robotpy/cxxheaderparser

fwiw, i was using pcpp (mentioned in cxxheaderparser) to preprocess all the headers (e.g., to fill out macros and things) then using cxxheaderparser to read the preprocessed files and generate bindings. had it not been for a handful of unsupported cases that i found running it through all the OCCT headers, i might have moved away from clang by now.

frmdstryr commented 3 years ago

If you comment out the if 'std::' in sig: lines and run the binder with the Test_Params.h file pybind can't figure out how to resolve the arguments.

It generates

cls_Test_Params.def("AddItems", (void (Test_Params::*)(const int &) const) &Test_Params::AddItems, "None", py::arg("items"));
cls_Test_Params.def("AddItems", (void (Test_Params::*)(const int &, const int &) const) &Test_Params::AddItems, "None", py::arg("items"), py::arg("indices"));

And gives this error:

/usr/bin/c++  -DpyBinderTest_EXPORTS -I../include -isystem /miniconda3/envs/smesh/include/python3.8 -isystem /miniconda3/envs/smesh/include -fPIC -fvisibility=hidden   -flto -fno-fat-lto-objects -MD -MT CMakeFiles/pyBinderTest.dir/output/Test.cxx.o -MF CMakeFiles/pyBinderTest.dir/output/Test.cxx.o.d -o CMakeFiles/pyBinderTest.dir/output/Test.cxx.o -c ../output/Test.cxx
../output/Test.cxx: In function ‘void pybind11_init_Test(pybind11::module_&)’:
../output/Test.cxx:76:90: error: no matches converting function ‘AddItems’ to type ‘void (class Test_Params::*)(const int&) const’
   76 | cls_Test_Params.def("AddItems", (void (Test_Params::*)(const int &) const) &Test_Params::AddItems, "None", py::arg("items"));
      |                                                                                          ^~~~~~~~
In file included from ../output/Test.cxx:25:
../include/Test_Params.h:16:10: note: candidates are: ‘void Test_Params::AddItems(const std::vector<const Test_Item*>&, const std::vector<int>&) const’
   16 |     void AddItems(const std::vector<const Test_Item*>& items, const std::vector<int>& indices) const {};
      |          ^~~~~~~~
../include/Test_Params.h:15:10: note:                 ‘void Test_Params::AddItems(const std::vector<const Test_Item*>&) const’
   15 |     void AddItems(const std::vector<const Test_Item*>& items) const {};
      |          ^~~~~~~~
../output/Test.cxx:77:103: error: no matches converting function ‘AddItems’ to type ‘void (class Test_Params::*)(const int&, const int&) const’
   77 | cls_Test_Params.def("AddItems", (void (Test_Params::*)(const int &, const int &) const) &Test_Params::AddItems, "None", py::arg("items"), py::arg("indices"));
      |                                                                                                       ^~~~~~~~
In file included from ../output/Test.cxx:25:
../include/Test_Params.h:16:10: note: candidates are: ‘void Test_Params::AddItems(const std::vector<const Test_Item*>&, const std::vector<int>&) const’
   16 |     void AddItems(const std::vector<const Test_Item*>& items, const std::vector<int>& indices) const {};
      |          ^~~~~~~~
../include/Test_Params.h:15:10: note:                 ‘void Test_Params::AddItems(const std::vector<const Test_Item*>&) const’
   15 |     void AddItems(const std::vector<const Test_Item*>& items) const {};

With the parameter signature change it generates

cls_Test_Params.def("AddItems", (void (Test_Params::*)(const std::vector<const Test_Item *> &) const) &Test_Params::AddItems, "None", py::arg("items"));
cls_Test_Params.def("AddItems", (void (Test_Params::*)(const std::vector<const Test_Item *> &, const std::vector<int> &) const) &Test_Params::AddItems, "None", py::arg("items"), py::arg("indices"));
trelau commented 3 years ago

Oh. I remember sometimes int showing up instead of the actual signature/type because something has gone wrong with clang (missing header, undefined type, etc.). If you see int showing up instead of the actual content then that's a good indicator that something went wrong during preprocessing (i.e., even before it gets to python side of things)

frmdstryr commented 3 years ago

If you see int showing up instead of the actual content then that's a good indicator that something went wrong during preprocessing

Most likely the case.. would it be make sense to try and detect this and error out?

trelau commented 3 years ago

Yes I think erroring out would be preferred. Maybe there is an clang compiler argument that needs to be set in the config file?