-
Notifications
You must be signed in to change notification settings - Fork 197
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
Use of -r/--remap flags where appropriate. #159
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment out of the scope of this PR. LGTM!
@@ -53,7 +53,7 @@ TransformListener::TransformListener(tf2::BufferCore & buffer, bool spin_thread) | |||
sstream << "transform_listener_impl_" << std::hex << reinterpret_cast<size_t>(this); | |||
rclcpp::NodeOptions options; | |||
// but specify its name in .arguments to override any __node passed on the command line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good, but I don't like the original motivation of this.
The node name uniqueness problem shouldn't be solved by generating a random name and overriding the command passed in the command line.
If someone wants to launch multiple TransformListener
, they should carefully remap the node name.
I'm not sure if that's possible, because TransformListener
isn't exactly a node. But in the case it's not possible, I think that problem should be solved instead of doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't weigh into the correctness of the workaround, just to keep this scoped. I'll defer to @rotu and @gonzodepedro, author and reviewer of #129 respectively, for that.
@ivanpauno How would remapping solve the problem of TransformListener’s name colliding with its parent node’s name? How would it help a parent node with multiple TransformListeners? Yes this is an ugly hack and I feel dirty for it. I welcome a better solution. |
Here’s the offending launch code: The |
Ok, now I understand the problem better. For the moment, it can be worked around in the launch file using |
Yes, but ideally the node of a TransformListener should be an implementation detail that the caller doesn't need to worry about. If the parent node has to both create a TransformListener object and reach inside to rename its nodes apart, that is terrible OOP design. Ideally, there should be no need for a subnode, or there should be no requirement for node name uniqueness, or you should be able to pass a flag to rclcpp that says "I don't care about the name of this node - make it unique for me".
Randomized names seem the lesser evil compared to moving the names of TransformListeners' subnodes to a launch file and manually giving them unique names - a detail that should be completely handled by the node itself. |
@rotu @ivanpauno I'd think this deserves a separate issue. I see valid arguments on both sides, and this is a completely unrelated PR that will hopefully soon be merged. |
Precisely what the title says.
Connected to ros2/rcl#491.