Remove Gazebo Classic support and Update for MoveIt Jazzy/Rolling

Kinovarobotics / ros2_kortex

ROS2 driver for the Gen3 Kinova robot arm

Other

51 stars 47 forks source link

Remove Gazebo Classic support and Update for MoveIt Jazzy/Rolling #228

Open sea-bass opened 4 months ago

sea-bass commented 4 months ago

This PR removes Gazebo Classic support, as this breaks builds, and fixes the existing (new) Gazebo support.

For more details, see https://github.com/Kinovarobotics/ros2_kortex/issues/217#issuecomment-2209595849

Critically:

sim_ignition arguments are gone, except for the calls to the macro from the ros2_robotiq_gripper repo... but I also have https://github.com/PickNikRobotics/ros2_robotiq_gripper/pull/54 to address this.
sim_gazebo arguments now refer to the "one and only" Gazebo

martinleroux commented 4 months ago

Hi @sea-bass , Thank you for your contribution. We will examine your pull request as soon as we have a ROS expert available to do so. In the meantime, I can however tell you that we require Humble builds to pass before we merge.

sea-bass commented 4 months ago

Hi @sea-bass , Thank you for your contribution. We will examine your pull request as soon as we have a ROS expert available to do so. In the meantime, I can however tell you that we require Humble builds to pass before we merge.

Yep, it seems the gz_ros2_control package is not being picked up by CI. Would appreciate a pointer on how to include repos from source, seems the .repos files didn't do it...

aalmrad commented 4 months ago

Hi @sea-bass, Thanks for the work you've put into this. After review, we believe that it would be preferable not to delete the Gazebo Classic support entirely. Instead, we would like to keep the support available but couple it with a warning for the users. This will provide an opportunity for those relying on this support to migrate to newer versions at their own pace. Your input has been helpful in identifying this part of the codebase that needs attention and we look forward to more of your contributions in the future.

sea-bass commented 4 months ago

Hi @sea-bass, Thanks for the work you've put into this. After review, we believe that it would be preferable not to delete the Gazebo Classic support entirely. Instead, we would like to keep the support available but couple it with a warning for the users. This will provide an opportunity for those relying on this support to migrate to newer versions at their own pace. Your input has been helpful in identifying this part of the codebase that needs attention and we look forward to more of your contributions in the future.

Sounds good. In that case, the gazebo_ros2_control package will have to be provided from source by anyone trying to use this package in Jazzy or later.

Of course, it's your choice, but this will make things harder for users to install on these later versions.

My recommendation would be to remove it from the main branch but keep it around on Iron and earlier branches.

moriarty commented 3 months ago

➕ yep probably best to create a branch where classic gazebo support is maintained and let main move forward and drop support.

Gazebo Classic is EOL in few months.

martinleroux commented 1 month ago

I am reviewing our open PRs. After some thought, I changed my mind and am inclined to agree with @moriarty after all. Once the builds are fixed, we can merge. @aalmrad , please see that a branch with gazebo-classic is left available outside of main.

aalmrad commented 1 month ago

Hello @sea-bass,

FYI a new branch called Gazebo-Classic-Support was created so that the Gazebo classic support can be removed on the main branch. After fixing the build issues, a merge can be done.

sea-bass commented 1 month ago

Fantastic, thank you! Once the builds are back up, I'll take a look to see if anything else is missing in this PR.