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 ns for depth image & pointcloud octomap updaters #2916

Merged
merged 1 commit into from
Nov 16, 2021

Conversation

Tuebel
Copy link
Contributor

@Tuebel Tuebel commented Oct 20, 2021

Description

Added a ns parameter to the octomap updaters in order to fix #2846 . This avoids the Tried to advertise a service that is already advertised errors.

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

@welcome
Copy link

welcome bot commented Oct 20, 2021

Thanks for helping in improving MoveIt and open source robotics!

@codecov
Copy link

codecov bot commented Oct 20, 2021

Codecov Report

Merging #2916 (baf1667) into master (a0ee202) will increase coverage by 0.02%.
The diff coverage is 85.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2916      +/-   ##
==========================================
+ Coverage   61.31%   61.33%   +0.02%     
==========================================
  Files         373      373              
  Lines       31742    31747       +5     
==========================================
+ Hits        19461    19469       +8     
+ Misses      12281    12278       -3     
Impacted Files Coverage Δ
...octomap_updater/src/pointcloud_octomap_updater.cpp 31.71% <85.72%> (+2.15%) ⬆️
...meterization/work_space/pose_model_state_space.cpp 81.14% <0.00%> (-1.88%) ⬇️
...raint_samplers/src/default_constraint_samplers.cpp 79.23% <0.00%> (+0.30%) ⬆️
...g_scene_interface/src/planning_scene_interface.cpp 49.72% <0.00%> (+1.16%) ⬆️
...ipulation/pick_place/src/manipulation_pipeline.cpp 71.30% <0.00%> (+1.86%) ⬆️
..._interface/src/detail/constrained_goal_sampler.cpp 76.00% <0.00%> (+2.00%) ⬆️

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 a0ee202...baf1667. Read the comment docs.

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

  • Almost trivial patch
  • Looks good
  • New functionality makes sense

thanks for the patch 👍

@v4hn v4hn mentioned this pull request Oct 22, 2021

std::string prefix = "";
if (!ns_.empty())
prefix = ns_ + "/";
Copy link
Member

Choose a reason for hiding this comment

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

My only concern is, we might end up with two or three slashes. For example, if the user defines these parameters:

ns = "/left_realsense/"
filtered_cloud_topic = "/camera_topic"

Copy link
Member

@AndyZe AndyZe Nov 15, 2021

Choose a reason for hiding this comment

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

Although I think that's an acceptable risk and shouldn't be too hard for a user to figure out by rostopic list / rosservice list. The amount of parsing code to handle that would be excessive IMO.

In short, I approve 👍

@AndyZe
Copy link
Member

AndyZe commented Nov 15, 2021

@Tuebel shall I merge it now?

@Tuebel
Copy link
Contributor Author

Tuebel commented Nov 16, 2021

Yes that would be great, thanks!

@v4hn v4hn merged commit 8a6c6ba into moveit:master Nov 16, 2021
@welcome
Copy link

welcome bot commented Nov 16, 2021

Congrats on getting your first MoveIt pull request merged and improving open source robotics!

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.

Error when using multiple DepthImageOctomapUpdater in sensors.yaml
3 participants