search

iris-ua / iris_lama_ros

LaMa on ROS
BSD 3-Clause "New" or "Revised" License
208 stars 59 forks source link

Remove backslash prefix from global ns topics #34

Closed cosmicog closed 3 years ago

cosmicog commented 3 years ago

Hello, when I tried to remap the pose topic of loc2d_ros node like this before: 1:

<remap from="pose" to="amcl_pose"/>

It didn't work so I need to do this instead: 2:

<remap from="/pose" to="amcl_pose"/>

People mostly advertise/subscribe-to global namespace topics without / prefix and remap like the first example above which is kinda habit for me too.

So I removed the backslash prefix from global namespace topics and services.

eupedrosa commented 3 years ago

Hi @cosmicog, sorry for the late answer.

People mostly advertise/subscribe-to global namespace topics without \ prefix and remap like the first example above which is kinda habit for me too.

I am a little confused, because, the first example is not a "remap" from the global namespace but rather from a local namespace.

Some of the topics that you are suggesting to change, for example /map and /initialpose are to be expected in the global namespace. RViz for example publishes to /initialpose.

Furthermore, applying the changes only to the loc2d_ros node, while we provide 2 more nodes, could create some incoherence in the package (if it does not existe already).

Let me know if this an important issue for you, or just a nice to have.

cosmicog commented 3 years ago

Actually, this change doesn't change the behaviour at all, those topics, (if the node is not namespaced like /robot0/loc2d_node) will appear in the global namespace since all topic names should start with /.

I am a little confused, because, the first example is not a "remap" from the global namespace but rather from a local namespace.

Exactly. I launch the node with ns="robot0" parameter in launch, and remap the topic like the first example while using with my updated version. Also the second one works without this pr too, with the same namespaced way in launch, but I didn't know that those topics were advertised in global namespace in the code and then I've read the code, then saw that I needed to change 1st to 2nd. Most of the ROS users & I are used to remap topics like the first example and advertise it in the code with 

global_nh_.advertise<some_msgs::some_type>("just_name_without_slash_in_the_beginning", ...)

Here's an example why we do this way:

ros::init(argc, argv, "my_node");
ros::NodeHandle priv_nh("~");
ros::NodeHandle nh;

// will be published as /my_node/my_priv_topic, (assuming node name is same in lauch)
// if namespaced, will be /robot0/my_node/my_priv_topic
priv_nh.advertise<some_msgs::SomeType>("my_priv_topic", ...);

// will be published as /my_topic1 always
nh.advertise<some_msgs::SomeType>("/my_topic1", ...); 

// will be published as /my_topic 
// if namespaced, will be /robot0/my_topic
nh.advertise<some_msgs::SomeType>("my_topic", ...);

Furthermore, applying the changes only to the loc2d_ros node, while we provide 2 more nodes, could create some incoherence in the package (if it does not existe already).

Since this doesn't change the published topics, they will still be published to /pose, /initialpose etc. If I would have more time, I would send a pr for those too. Or feel free to just remove back slashes from global namespace topics later like i did in this pr.

Let me know if this an important issue for you, or just a nice to have.

I'm currently using my fork from master branch in Noetic. After this PR is accepted, I'll open another PR for merging master into noetic-devel branch. I already checked what was different in other nodes and rest in master vs noetic-devel, and saw that tf links are different(starting with / which is wrong in Noetic too). This might be the reason why the deb from repos ros-noetic-iris-lama-ros didn't work and I needed to fork master in the beginning to test it.

cosmicog commented 3 years ago

This way, if not namespaced;

<node pkg="iris_lama_ros" type="loc2d_ros" name="loc2d_ros"/>

It will work exactly like before and publish to /pose

But if namespaced;

<node pkg="iris_lama_ros" type="loc2d_ros" name="loc2d_ros" ns="robot0"/>

It will allow e.g. launching multiple robots in gazebo with i.e. /robot0/pose, /robot1/pose topics.

eupedrosa commented 3 years ago

You are using namespaces, now it makes more sense. And for some reason, I was thinking that the PR was also changing nh to priv_nh, and that is not the case.

I see no problem now in accepting the PR:

I'm currently using my fork from master branch in Noetic. After this PR is accepted, I'll open another PR for merging master into noetic-devel branch. I already checked what was different in other nodes and rest in master vs noetic-devel, and saw that tf links are different(starting with / which is wrong in Noetic too). This might be the reason why the deb from repos ros-noetic-iris-lama-ros didn't work and I needed to fork master in the beginning to test it.

Thank you for this. I will be expecting the PR.