Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pybind11 manual bindings #499

Merged
merged 18 commits into from
Jul 21, 2019
Merged

Pybind11 manual bindings #499

merged 18 commits into from
Jul 21, 2019

Conversation

gilwoolee
Copy link
Contributor

This PR contains minimal class definitions for exposing aikido classes needed for other repositories such as libherb and pr_tsr.

The python binding is built not part of aikido but as a separate project after aikido is built via catkin.


Before creating a pull request

  • Document new methods and classes
  • Format code with make format

Before merging a pull request

  • Set version target by selecting a milestone on the right side
  • Summarize this change in CHANGELOG.md
  • Add unit test(s) for this change

@gilwoolee gilwoolee added the pr: work-in-progress Calls for preliminary review of the PR. Remove when PR is ready for complete review. label Feb 3, 2019
@codecov
Copy link

codecov bot commented Feb 3, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@de5ee11). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #499   +/-   ##
=========================================
  Coverage          ?   77.85%           
=========================================
  Files             ?      262           
  Lines             ?     6768           
  Branches          ?        0           
=========================================
  Hits              ?     5269           
  Misses            ?     1499           
  Partials          ?        0

Copy link
Contributor

@brianhou brianhou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please delete the .swp file committed in b0691c6

Copy link
Contributor

@brianhou brianhou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly left comments on the README. Can you also explain what the difference is between aikidopy.cpp and aikido_py.cpp?

python/README.md Outdated Show resolved Hide resolved
python/README.md Outdated Show resolved Hide resolved
python/README.md Outdated Show resolved Hide resolved
python/README.md Outdated Show resolved Hide resolved
python/README.md Outdated Show resolved Hide resolved
python/README.md Outdated Show resolved Hide resolved
python/README.md Outdated Show resolved Hide resolved
python/README.md Outdated Show resolved Hide resolved
python/README.md Outdated Show resolved Hide resolved
python/README.md Outdated Show resolved Hide resolved
@gilwoolee
Copy link
Contributor Author

Mostly left comments on the README. Can you also explain what the difference is between aikidopy.cpp and aikido_py.cpp?

Hmm I was following @jslee02 's example code I got for herbpy so @jslee02 might have a better answer (or suggestions). My understanding is that aikidopy.cpp defines things (more like hpp files) and aikido_py.cpp actually implements them. @jslee02 could you clarify?

@jslee02
Copy link
Member

jslee02 commented Feb 11, 2019

I use <python_module_name>.cpp for the main module (aikidopy.cpp in this case), which contains PYBIND11_MODULE. For the actual bindings, I define them in separate files per namespace and classes for example (see: https://github.com/dartsim/dart/tree/pybind11/python/dartpy).

It seems aikido_py.cpp is (currently) a single file for all the bindings of AIKIDO. I think, if necessary, it could be split and renamed when more AIKIDO APIs are bound.

Copy link
Member

@jslee02 jslee02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current CMake script seems to be meant to build aikidopy assuming AIKIDO is already installed. If that's the case, I suggest having adikiopy in a separate repo. Otherwise, please:

  • remove find_package(DART ...), which is already called, and the variables (i.e., DART_INCLUDE_DIRS and DART_LIBRARIES)
  • remove find_package(AIKIDO ...), and use the necessary targets (e.g., aikido-common, aikido-statespace, and so on).

@jslee02 jslee02 requested a review from egordon as a code owner July 20, 2019 14:53
@jslee02 jslee02 requested a review from yskim041 as a code owner July 20, 2019 14:53
@jslee02 jslee02 removed the pr: work-in-progress Calls for preliminary review of the PR. Remove when PR is ready for complete review. label Jul 20, 2019
@brianhou brianhou merged commit 2c75322 into master Jul 21, 2019
@brianhou brianhou deleted the pybind11_manual branch July 21, 2019 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants