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

Added Python interfaces to some Ignition Gazebo methods #1219

Merged
merged 37 commits into from Feb 25, 2022

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Nov 19, 2021

Signed-off-by: ahcorde ahcorde@gmail.com

🎉 New feature

Summary

Added Python interfaces to same Ignition Gazebo methods with pybind11

Requires gazebosim/gz-math#280

Related with this issue #789 (comment)

Test it

I created an example examples/python/helperFixture.py.

Export your python path:

export PYTHONPATH=$PYTHONPATH:<your workspace>/install/lib/python/

Run the example:

python3 <your workspace>/src/ign-gazebo/examples/python/testFixture.py

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde self-assigned this Nov 19, 2021
@osrf-triage osrf-triage added this to Inbox in Core development Nov 19, 2021
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Nov 19, 2021
@codecov
Copy link

codecov bot commented Nov 19, 2021

Codecov Report

Merging #1219 (d5e34d5) into ign-gazebo6 (3b94bd2) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gazebo6    #1219   +/-   ##
============================================
  Coverage        62.93%   62.93%           
============================================
  Files              299      299           
  Lines            24167    24167           
============================================
  Hits             15210    15210           
  Misses            8957     8957           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b94bd2...d5e34d5. Read the comment docs.

@chapulina chapulina added the scripting Scripting interfaces to Ignition label Nov 19, 2021
@chapulina chapulina moved this from Inbox to In review in Core development Nov 19, 2021
@chapulina chapulina changed the title Added Python interfaces to same Ignition Gazebo methods Added Python interfaces to some Ignition Gazebo methods Nov 19, 2021
@chapulina chapulina added needs upstream release Blocked by a release of an upstream library enhancement New feature or request labels Nov 19, 2021
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Great work, works as advertised! At this first pass, I just have some high-level questions about the implementation.

You can ignore the style nitpicks for now and address them once we've settled on the approach.

python/src/ignition/gazebo/destroyable.hh Outdated Show resolved Hide resolved
python/src/ignition/gazebo/destroyable.hh Outdated Show resolved Hide resolved
python/src/ignition/gazebo/entity_component_manager.cc Outdated Show resolved Hide resolved
python/src/ignition/gazebo/entity_component_manager.hh Outdated Show resolved Hide resolved
python/src/ignition/gazebo/entity_component_manager.hh Outdated Show resolved Hide resolved
python/src/ignition/gazebo/helper_system.hh Outdated Show resolved Hide resolved
python/src/ignition/gazebo/helper_system.hh Outdated Show resolved Hide resolved
python/src/ignition/gazebo/server.hh Outdated Show resolved Hide resolved
python/src/ignition/gazebo/world.cc Outdated Show resolved Hide resolved
examples/python/helperFixture.py Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Just some minor stylistic comments, I think we're almost there!

It would be good to have a little test for this on CI to make sure that it's working once we release ign-math debs.

python/src/ignition/gazebo/server_config.cc Outdated Show resolved Hide resolved
python/src/ignition/gazebo/server_config.hh Outdated Show resolved Hide resolved
python/src/ignition/gazebo/world.cc Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from chapulina January 7, 2022 08:42
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Can we add the python dependencies to packages.apt so that the bindings are built on CI?

python/src/ignition/gazebo/EntityComponentManager.hh Outdated Show resolved Hide resolved
python/src/ignition/gazebo/HelperSystem.hh Outdated Show resolved Hide resolved
tutorials/python_interfaces.md Outdated Show resolved Hide resolved
python/src/ignition/gazebo/World.cc Show resolved Hide resolved
python/src/ignition/gazebo/Destroyable.hh Outdated Show resolved Hide resolved
python/CMakeLists.txt Outdated Show resolved Hide resolved
python/CMakeLists.txt Outdated Show resolved Hide resolved
python/CMakeLists.txt Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Jan 11, 2022

I made some changes that might be conflictive.

  • EventManager: Changed unique_ptr to shared_ptr in a private member to be able to copy the class
  • TextureFixture: Changed some reference to pointers
    • some integration tests have some changes: Spherical_coordinates and thruster

I can open a different PRs, these PRs might require to tick-tock some of the methods

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

It's looking great! I just have final suggestions in #1308 and after that I'm ready to approve! 🎉

chapulina and others added 2 commits January 25, 2022 08:45
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Yay! This is great! Now we just need an ign-math6 stable release for the tests to pass.

@chapulina
Copy link
Contributor

I tested this branch against ign-math6.9.3~pre2 debs, and it works well! 🎉

@chapulina chapulina mentioned this pull request Jan 25, 2022
7 tasks
@chapulina
Copy link
Contributor

A couple of problems in Jenkins CI.

  1. The test fails (but it passes on GitHub actions)
213: Test command: /usr/bin/python3 "/home/jenkins/workspace/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/ign-gazebo/python/test/testFixture_TEST.py"
213: Environment variables: 
213:  PYTHONPATH=/usr/lib/python/
213:  LD_LIBRARY_PATH=/usr/lib:/usr/local/lib/x86_64-linux-gnu
213: Test timeout computed to be: 1500
213: Traceback (most recent call last):
213:   File "/home/jenkins/workspace/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/ign-gazebo/python/test/testFixture_TEST.py", line 19, in <module>
213:     from ignition.common import set_verbosity
213: ModuleNotFoundError: No module named 'ignition.common'
213/213 Test #213: testFixture_TEST.py ....................................***Failed    0.23 sec
  1. Jenkins doesn't report the failure (CC @j-rivero )

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor

chapulina commented Jan 28, 2022

For packaging, we'll need to do the same as gazebo-release/gz-math6-release#6 for ign-gazebo6-release and ign-gazebo7-release. @j-rivero said he can help in the coming days. The plan is to install the ign-common and sdformat bindings as if they came from those packages, and if we ever move the bindings there, we should be able to flip the switch without affecting users.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Copy link
Contributor

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

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

Patch seems to work as expected, tested via patch in release repo here:

-- Installing: /home/jenkins/workspace/ign-gazebo6-debbuilder/build/ignition-gazebo-6.6.0~pre1/debian/tmp/usr/lib/python3/dist-packages/sdformat.cpython-38-x86_64-linux-gnu.so
-- Installing: /home/jenkins/workspace/ign-gazebo6-debbuilder/build/ignition-gazebo-6.6.0~pre1/debian/tmp/usr/lib/python3/dist-packages/ignition/gazebo.cpython-38-x86_64-linux-gnu.so
-- Set runtime path of "/home/jenkins/workspace/ign-gazebo6-debbuilder/build/ignition-gazebo-6.6.0~pre1/debian/tmp/usr/lib/python3/dist-packages/ignition/gazebo.cpython-38-x86_64-linux-gnu.so" to ""

@ahcorde ahcorde merged commit ac989f8 into ign-gazebo6 Feb 25, 2022
Core development automation moved this from In review to Done Feb 25, 2022
@ahcorde ahcorde deleted the ahcorde/python/gazebo branch February 25, 2022 19:22
@scpeters scpeters moved this from Done to Highlights in Core development Feb 28, 2022
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-25-fortress-edifice-citadel/1343/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 🏯 fortress Ignition Fortress needs upstream release Blocked by a release of an upstream library scripting Scripting interfaces to Ignition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants