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.

Sign up for GitHub

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

Jump to bottom

Port the collisions_updater to ROS2 #732

Merged
merged 2 commits into from
Oct 12, 2021
Merged

Port the collisions_updater to ROS2 #732

merged 2 commits into from
Oct 12, 2021

Conversation

brennand
Copy link
Contributor

@brennand brennand commented Oct 11, 2021 •
edited
Loading

Description

This is a port for the collision updater:

  1. Collision updater 100% working in ROS2
  2. removed the launch file as its for ROS1 and it launches the GUI which hasn't been ported yet. I decided to remove it so that people are not confused and try and run it.
  3. Added comment in the README of how to run the command line tool
  4. Striped all the GUI "stuff" out of the CMake as it will have to be re-written when the GUI is ported.
  5. changed from .htp .hpp as that seems to be the ROS2 way?

linked against this issue:

#724

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@brennand brennand changed the title WIP Bren/collisions updater ROS2 Port of moveit_setup_assistant Oct 11, 2021
@codecov
Copy link

codecov bot commented Oct 11, 2021 •
edited
Loading

Codecov Report

Merging #732 (2ffaf9a) into main (c6384e9) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #732      +/-   ##
==========================================
- Coverage   54.24%   54.22%   -0.02%     
==========================================
  Files         192      192              
  Lines       20230    20230              
==========================================
- Hits        10972    10967       -5     
- Misses       9258     9263       +5
Impacted Files Coverage Δ
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 72.84% <0.00%> (-1.88%) ⬇️

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 c6384e9...2ffaf9a. Read the comment docs.

tylerjw
tylerjw reviewed Oct 12, 2021
View reviewed changes
moveit_setup_assistant/CMakeLists.txt Outdated Show resolved Hide resolved
moveit_setup_assistant/CMakeLists.txt Outdated Show resolved Hide resolved
moveit_setup_assistant/CMakeLists.txt Outdated Show resolved Hide resolved
moveit_setup_assistant/CMakeLists.txt Outdated Show resolved Hide resolved
moveit_setup_assistant/launch/setup_assistant.launch Outdated Show resolved Hide resolved
moveit_setup_assistant/CMakeLists.txt Show resolved Hide resolved
@brennand brennand force-pushed the bren/collisions_updater branch 2 times, most recently from c6e7b40 to 9d0a1f1 Compare October 12, 2021 16:45
@brennand brennand requested a review from tylerjw October 12, 2021 16:45
@brennand brennand requested a review from tylerjw October 12, 2021 18:54
@brennand brennand force-pushed the bren/collisions_updater branch 4 times, most recently from 62b6b00 to 8eb90ca Compare October 12, 2021 20:54
@brennand
Ported the collisions_updater to ROS2
6fb714f 
* Ported the collision updater from ros1 to ros2 in the moveit_setup_assistant
* Added README instrutions on how to use the command line tool
* Changed the include from .h to .hpp inline with ROS2 standard
tylerjw
tylerjw approved these changes Oct 12, 2021
View reviewed changes
Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

Assuming this passes CI it looks good to me. Thank you!

@tylerjw tylerjw changed the title ROS2 Port of moveit_setup_assistant Port the collisions_updater to ROS2 Oct 12, 2021
@tylerjw tylerjw merged commit b6f9e49 into moveit:main Oct 12, 2021
@brennand brennand deleted the bren/collisions_updater branch October 13, 2021 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JafarAbdi JafarAbdi JafarAbdi left review comments

@tylerjw tylerjw tylerjw approved these changes

@MarqRazz MarqRazz Awaiting requested review from MarqRazz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@brennand @tylerjw @JafarAbdi