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

Makes rviz trajectory visualization topic relative #2835

Merged
merged 3 commits into from
Aug 24, 2021

Conversation

rickstaa
Copy link
Contributor

@rickstaa rickstaa commented Aug 24, 2021

This commit makes sure the RVIZ trajectory visualization plugin looks for the trajectory topic to be on the relative ns instead of the absolute ns when it is first added to Rviz (i.e. move_group/display_planned_path vs. /move_group/display_planned_path). This was done to make sure that it respects the namespaces that are set in launch files. This is useful when you are using MoveIt for multi robot control.

I did not find any images or text in the documentation that needs to be updates. Please let me know if I missed something.

This commit makes the trajectory visualization topic relative. This was
done to make sure that it respects the namespaces that are set in
launch files.

Updates MIGRATION document
@welcome
Copy link

welcome bot commented Aug 24, 2021

Thanks for helping in improving MoveIt and open source robotics!

Copy link
Contributor

@felixvd felixvd left a comment

Choose a reason for hiding this comment

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

LGTM and it looks like it should work, but I didn't test

MIGRATION.md Outdated Show resolved Hide resolved
Co-authored-by: Felix von Drigalski <FvDrigalski@gmail.com>
@rickstaa
Copy link
Contributor Author

@felixvd Thanks for looking into my pull request. I applied the changes you requested. I tested this pull request using the following steps:

  1. Build the MoveIt package from source while bing on the pull request branch.
  2. Start a ROS core.
  3. Start RVIZ.
  4. Add the trajectory plugin.

I did not encounter any errors while performing this steps.

@felixvd
Copy link
Contributor

felixvd commented Aug 24, 2021

Does it display the trajectory like in this tutorial? :)

@rickstaa
Copy link
Contributor Author

rickstaa commented Aug 24, 2021

@felixvd Ah that's a good tutorial to test it on. It seems to work, see GIF below. There are some warnings shown under the MarkerArray, but these look unrelated to my pull request as they also are present on the master branch.

proof

Copy link
Contributor

@felixvd felixvd left a comment

Choose a reason for hiding this comment

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

Thanks for double checking!

@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #2835 (dc43240) into master (7cea1f2) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head dc43240 differs from pull request most recent head 40f9adc. Consider uploading reports for the commit 40f9adc to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2835      +/-   ##
==========================================
+ Coverage   60.81%   60.83%   +0.02%     
==========================================
  Files         366      366              
  Lines       31717    31717              
==========================================
+ Hits        19285    19291       +6     
+ Misses      12432    12426       -6     
Impacted Files Coverage Δ
...g_scene_interface/src/planning_scene_interface.cpp 47.46% <0.00%> (-1.12%) ⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp 68.13% <0.00%> (-0.13%) ⬇️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 78.25% <0.00%> (+0.39%) ⬆️
...meterization/work_space/pose_model_state_space.cpp 81.14% <0.00%> (+0.63%) ⬆️
...on/pick_place/src/approach_and_translate_stage.cpp 74.03% <0.00%> (+0.65%) ⬆️
.../ompl_interface/src/detail/constrained_sampler.cpp 59.46% <0.00%> (+16.22%) ⬆️

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 7cea1f2...40f9adc. 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.

Obvious fix, thank you. I wouldn't even mention it in the MIGRATIONS.md, but as you want it there, I just added another guard so most people can skip it when they look at the list :)

MIGRATION.md Outdated Show resolved Hide resolved
@v4hn v4hn merged commit bef8f7c into moveit:master Aug 24, 2021
@welcome
Copy link

welcome bot commented Aug 24, 2021

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

@rickstaa rickstaa deleted the changes_rviz_visualization_topic branch August 24, 2021 23:19
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.

None yet

3 participants