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

Add python bindings based on Pyside2 #889

Open
wants to merge 17 commits into
base: rolling
Choose a base branch
from

Conversation

hellantos
Copy link

@hellantos hellantos commented Aug 28, 2022

This PR will add python bindings for rviz using Pyside2 for a couple of classes. The goal is to make rviz usable as a qwidget in python.

Some design decisions:

  • Why Pyside2 and not SIP? Honestly, documentation for Pyside is much better and resources are easier found. Plus the license of Pyside is much more business friendly.
  • Why have class VisualizerFramePy? It seems super complicated to wrap WeakPtr, or at least I could not get it to work. So VisualizerFramePy has the ROS Interface integrated and one does not need to pass it in. Open to help here.
  • This PR will not target Windows. I just do not have the knowledge. But it should be possible to do the generation for Windows as well.
  • We are using this in a proof of concept project only - so testing will be limited.

Open points:

  • Restructuring and tidying up
  • Documentation on how to use
  • Add more necessary classes
  • Testing
  • Add correct dependencies to package.xml
  • Add pyside and shiboken to rosdistro systemdeps

Christoph Hellmann Santos added 2 commits August 24, 2022 23:42
Signed-off-by: Christoph Hellmann Santos <christoph.hellmann.santos@ipa.fraunhofer.de>
@clalancette
Copy link
Contributor

This PR should wait for the conclusion of ros-visualization/python_qt_binding#114 before proceeding. But I just wanted to make one point:

  • This PR will not target Windows. I just do not have the knowledge. But it should be possible to do the generation for Windows as well.

Unfortunately, Windows support must get completed before we can consider merging this PR. Windows is one of our Tier-1 platforms, and so features that go in must also support it. Hopefully when we resolve the issue above this will become easier.

@hellantos
Copy link
Author

Unfortunately, Windows support must get completed before we can consider merging this PR. Windows is one of our Tier-1 platforms, and so features that go in must also support it. Hopefully when we resolve the issue above this will become easier.

Okay. Problem is, I am not really equipped to test this on Windows, as I am not to familiar with ROS2 on Windows. It would cost me a serious amount of time to do that. Generally, it would only be a thing of getting the correct paths for shiboken and pyside Libs and Execs into CMake - I think I can manage that. There is a python script for Pyside by Qt that is thought for Windoof and Linux to extract the necessary info and an example CMakeLists.txt. But testing this on Windows and managing the dependencies plus doing potential changes is something I cannot do right now.

I see 2 options:

  1. Move rviz_python_bingings package to a seperate repo - and think about merging it in here later. At least people can start using it.
  2. Someone with time and a ROS2 on Windows setup helps with testing and implementing potential changes.

@hellantos
Copy link
Author

hellantos commented Aug 30, 2022

@clalancette
Okay, I have added intial Windows support -- to the best of my knowledge based on the examples provided by qt -- not tested.

Windows Dependencies: People will need to install pyside2 shiboken2 and shiboken2-generator from qt's release repository using pip. Instructions in readme. They will also need to add the installed dlls to PATH. If not we'll need to copy them to CMAKE_INSTALL_PREFIX/lib in a deployment step - this is kinda shady. Any opinions here?

pip install \
    --index-url=https://download.qt.io/official_releases/QtForPython/ \
    --trusted-host download.qt.io \
    shiboken2 pyside2 shiboken2_generator

Does anyone have a windows setup to test this? It is really hard to set this up in my company due to IT restrictions...

@hellantos hellantos marked this pull request as ready for review August 30, 2022 12:02
@hellantos
Copy link
Author

This depends on ros/rosdistro#34243. Once merged checks should pass.

@hellantos hellantos mentioned this pull request Feb 10, 2023
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.

3 participants