Add tf prefix helper and test #2803
Conversation
github-actions
bot
requested review from
bmagyar,
destogl,
erickisos,
harderthan,
mcbed,
moriarty and
olivier-stasse
| if (!tf_prefix_enabled) | ||
| { | ||
| return std::string{}; | ||
| } |
There was a problem hiding this comment.
Do we need to have this arg here? Can't we condition it outside whoever is using this code?
There was a problem hiding this comment.
IMHO, the helper methods shouldn't condition things, they do some standard operation and the users adapt to their usecases
There was a problem hiding this comment.
this refers to my comment here: ros-controls/ros2_controllers#1997 (comment)
The current state feels a bit unconvenient. If !tf_prefix_enabled then it is just the given odom frame without adding something. How would you design this?
There was a problem hiding this comment.
How about something like this
If the tf_prefix is empty, then return empty
If the tf_prefix is ~, then then the node namespace
If the tr_pedix is anything else, then return that
Obviously, trimming the /'s
What do you think about it?
There was a problem hiding this comment.
IMO, the previous version -helper without the prefix enable arg- was causing clutter when called with the ternary operator. It was still a one liner but looked more complicated then it is. I can remove it again regardless so it can comply the standard:
the helper methods shouldn't condition things
The decision is yours.
If the tf_prefix is empty, then return empty
If the tf_prefix is ~, then then the node namespace
If the tr_pedix is anything else, then return that
Not clear about this second one, do you mean i take node namespace if the user inputs the tilde character specifically or is it a result of some operation I am not aware?
There was a problem hiding this comment.
Yes, if the user inputs tilde, you replace that part with node namespace. Wouldn't that work?. This way you wouldn't need the parameter of tf_prefix_enable or not right?
For the conditioning part, the part you say complicated. I need to check that part of the code and get back to you.
There was a problem hiding this comment.
Previous one I tested here
Thanks for the reference. My question is do we really need tf_frame_prefix_enable parameter? If so, why?
christophfroehlich
changed the title
3 participants
Added the tf prefix helper here instead of control_toolbox. Prefix enabler flag re-added and frame removed from input arguments.
Related PR