-
Notifications
You must be signed in to change notification settings - Fork 695
Fix deprecations in image_common and tf2_ros #3567
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3567 +/- ##
==========================================
+ Coverage 0.00% 46.22% +46.22%
==========================================
Files 180 720 +540
Lines 22601 62743 +40142
Branches 3099 7594 +4495
==========================================
+ Hits 0 28995 +28995
- Misses 22601 33582 +10981
- Partials 0 166 +166 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
729a606 to
3f931ed
Compare
|
Still remaining deprecation warning treated as error failures related to #3528 |
|
This is building successfully locally now with I have not tested that Rviz functionality is preserved with this change - I am a little concerned about any |
|
|
||
| if (planning_scene_monitor_) | ||
| updateInternal(std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::duration<float>(wall_dt)), | ||
| std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::duration<float>(ros_dt))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should just have this call PlanningSceneDisplay::update(std::chrono::nanoseconds wall_dt, std::chrono::nanoseconds ros_dt) rather than duplicate the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea when implementing ros2/rviz#1533 was that the update(float, float) methods could be removed in favour of the update(std::chrono... methods.
If the update(float, float) overload is removed, then calling update(float, float) method will just call the base class version, which automatically calls the (overloaded) update(std::chrono... version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I am keeping this overloaded PlanningSceneDisplay::update(float wall_dt, float ros_dt) method is due to how we use branches in the moveit2 repo:
humble,kilted,jazzy, etc - Stable versions of non-EOL distros. No API breaking changes are pushed to these branches. Build farm releases for non-rolling distros are built from these branches.main- This targetsrolling, but also compiles against non-EOL distro APIs with MoveIt breaking changes. This enables people on Humble to use the latest features that broke API if they wish (but requires a source build).
The changes to motion_planning_display.hpp might make this more clear.
So I'm keeping the MoveIt Rviz plugin PlanningSceneDisplay::update(float wall_dt, float ros_dt) API to avoid breaking API for those folks (but have marked it as deprecated in Rolling).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be sufficient to just add a using declaration to each class, to use the base class' update methods?
Something like
class PlanningSceneDisplay : public rviz_common:Display {
...
using rviz_common::Display::update;
void update(std::chrono::nanoseconds ...
}
so that PlanningSceneDisplay::update(float, float) just calls rviz_common::Display(float, float) (which in turn just calls PlanningSceneDisplay::update(std::chrono, std::chrono))?
(I think the using declaration is needed to avoid shadowing / name hiding of the update(float, float) method with the update(std::chrono method. See https://stackoverflow.com/questions/31271661/partial-inheritance-of-set-of-overloaded-virtual-functions for details.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I am following, let's give it a shot.
If I have a rough time with it, I may just revert to the more verbose approach so I can get this fix in (and fix CI).
moveit_ros/visualization/robot_state_rviz_plugin/src/robot_state_display.cpp
Outdated
Show resolved
Hide resolved
moveit_ros/visualization/trajectory_rviz_plugin/src/trajectory_display.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think .hpp is also available in Kilted and older, do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which .hpp file do you mean? the tf2_ros .hpp files? That change only went in July, so the "treat deprecations as errors" compile failures only need to be addressed in Rolling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the tf2 ros hpp files, it is also backported and now available in Kilted and order, so I think it's fine for older distribution to use .hpp as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing out using tf2_ros hpp includes in Kilted now.
| { | ||
| if (int_marker_display_) | ||
| int_marker_display_->update( | ||
| std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::duration<float>(wall_dt)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this will treat float wall_dt and float ros_dt as being in seconds, and convert them to std::chrono::nanoseconds?
The point of ros2/rviz#1533 is that float wall_dt and float ros_dt are (erroneously, and contrary to documentation) already in nanoseconds. See ros2/rviz#1322 and #3528.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review - I will read through those issues in more detail and revise!
*.h headers is deprecated into tf2 libs . -> switch to *.hpp headers
80d4883 to
2eeff3a
Compare
|
Unfortunately we will need the next rolling release (which will include xacro 2.1.1) for this to pass as is (see PickNikRobotics/launch_param_builder#16). |
|
@rhaschke Looking for some advice here. I'm trying to get the required Rolling CI jobs green in this PR so we can merge in PRs without a admin overriding CI (and also not knowing if there are errors past the deprecation-treated-as-error failures). What would you recommend with regard to PickNikRobotics/launch_param_builder#16 ?
|
|
https://discourse.openrobotics.org/t/preparing-for-rolling-sync-2025-09-26/50259 Looks like the sync has begun |
|
Sync has happened |
|
Looks like the pipelines are still failing with xacro and warehouse problems |
|
Xacro 2.1.1 is released on rolling. And should fix the pathlib compatibility. The code shown in the stacktrace doesn't match the 2.1.1 tag. So maybe caching is preventing the newest xacro to be installed. |
|
Yeah, it looks like the old dependencies are being pulled in from the docker cache https://github.com/moveit/moveit2/actions/caches The quick fix would just be to delete the relevant caches from there. The longer fix would be to have a preprocessing step so the version numbers are passed into the relevant places in the dockerfile, so it rebuilds the correct layers when dependencies update. |
|
Looks like deleting the rolling and humble ci cache did not get it passing...maybe I need to delete it all. |
|
I think this is actually a correct deprecation treated as error failure, though I don't understand how things were passing earlier.
|
|
Oh, the constructor deprecation was in ros2/geometry2#714 not ros2/geometry2#805, that is how. 😭 |
|
Yeah, looks like new errors. So at least the rolling sync problems are fixed. |
|
Also need to fix deprecations in ros2/rclcpp#2848 now. It would be nice if the deprecation warnings linked to the PR that generated them. It would save me some time chasing stuff down, especially because the ROS 2 SEO still needs a lot of attention. |
…ode_interfaces::NodeBaseInterface::SharedPtr node_ptr) to SingleThreadedExecutor::spin_some() API
|
It looks like the |
|
moveit/moveit2_tutorials#1071 should get |
Description
This PR gets the required CI jobs passing again by addressing API changes from the following:
I check which header format / API to use by checking the rclcpp version, which is generated via rclcpp'ss invocation of the
ament_generate_version_headerCMake function. Other places in the MoveIt2 codebase where the include could be .h or .hpp (depending on the target distro), we decide which to use by checking if the hpp file can be found. I decided to just useRCLCPP_VERSION_GTEeverywhere and make a note which newest distro uses the "old" API so we can delete those checks when that distro (for example, Kilted) goes EOL.I am following the
RCLCPP_VERSION_GTEversions used in the Nav2 project, which are pretty close to what I saw when inspecting the rclcpp package version at each distro branch date.elseif)Caveats
parallel_gripper_controllerreplacedgripper_controllersament_target_dependencies()in the Rolling jobChecklist