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 TF Prefix to effort plugin #1213

Merged
merged 5 commits into from
May 10, 2018

Conversation

ahoarau
Copy link
Contributor

@ahoarau ahoarau commented Mar 30, 2018

Simple PR to add a TF Prefix property to the Effort Plugin.
Use case : /robot1/robot_description + tf_prefix set to robot1

Also fixes #892.

@dhood dhood added this to the first melodic release milestone Apr 6, 2018
@dhood
Copy link
Contributor

dhood commented Apr 27, 2018

Thanks for the patch, @ahoarau

@tfoote I have read that the tf prefix was causing headaches in tf1, and I've seen errors before about e.g. base_link not resolving to /base_link well. Would you be able to comment on the impact of prepending a forward slash to a frame where it didn't otherwise have one? I'm thinking we should only add the slash if the tf_prefix is specified, to minimise impact.

@ahoarau
Copy link
Contributor Author

ahoarau commented Apr 27, 2018

You are absolutely right, we should not add leading slash if no tf_prefix is present.
Let's change it for :

std::string tf_prefix = tf_prefix_property_->getStdString();
std::string parent_link_name =  (tf_prefix.empty() ? "" : tf_prefix + "/" ) + joint->child_link_name;

Very glad to hear that tf_prefix is meant to disappear, but I'm not sure how I will spawn multiple robots in the same master. Multimaster is quite a big overhead :)

@dhood
Copy link
Contributor

dhood commented Apr 27, 2018

That change looks reasonable, but for clarity the variable should now be named tf_frame_id as opposed to parent_link_name

btw this change will be held for ROS Melodic since it will cause an ABI change

@dhood
Copy link
Contributor

dhood commented Apr 27, 2018

I broke out the non-abi breaking change into #1229 so it can be released into indigo/kinetic

@dhood dhood changed the base branch from kinetic-devel to melodic-devel April 27, 2018 23:04
@dhood
Copy link
Contributor

dhood commented Apr 30, 2018

@ros-pull-request-builder retest this please.

@wjwwood
Copy link
Member

wjwwood commented May 10, 2018

@dhood merge or comment

@dhood dhood merged commit d9afa34 into ros-visualization:melodic-devel May 10, 2018
@dhood dhood changed the title add TF Prefix to effort plugin and fix #892 Add TF Prefix to effort plugin May 10, 2018
@dhood
Copy link
Contributor

dhood commented May 10, 2018

thanks for iterating @ahoarau

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash on Effort plugin
3 participants