Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update remapping order of __ns and __node to not affect each other #299
Update remapping order of __ns and __node to not affect each other #299
Changes from 1 commit
873ada9
93a1b51
520d649
31ca246
33770a1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
what if user wants to remap
talker
into namespaceA
andB
? I believe this is really common use case. I am not so convinced to change order. I thinkNode name -> Namespace
would be understandable and usable for user.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.
Thanks for your response. @fujitatomoya
Sorry, I don't catch it. How can we remap one node
talker
into two different namespacesA
andB
?It has a limitation.
Imageing there are two nodes
publisher_node
andsubscriber_node
in an application./publisher_node
/subscriber_node
I want to remap these two nodes as following (ros2/rcl#806),
/publisher_node
-----> '/ns1/aaa'/subscriber_node
-----> '/ns2/aaa'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.
Maybe this case is suited for the real world.
/left_camera -----> /left/camera
/right_camera -----> /right/camera
It seems we can not remap them with current remapping order.
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.
bad example, ignore it... sorry😢
I don't see this is because of remapping order, but implementation. I think we should have some kinda justification to change order to support two nodes with the same name but in different namespaces.
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.
That's right, users can set two nodes with Node("camera", "/left") and Node("camera", "/right") in implementation.
but we can't ask users to do this, if somebody just wants to use remapping, there is a limitation.
Actually, the sample about the 'camera' above is the reason why I open this issue.
I found no discussion about why ros2 use this kind of remapping order(node name -> namespace), after checking ros2/rcl#217 and #181, it seems that ros2 follow the same implementation order to ros1. (If I remember correctly, ros1 doesn't have the remapping order for __name and __ns because it just use one node in an application. )