Enable private names for InteractiveMarkerServer

[LeroyR]

remove topic_ns member and instead pass the parameter to the node_handle constructor
This enables to use Private Names e.g.
interactive_markers::InteractiveMarkerServer marker_server("~/name")

[LeroyR]

Dont really get what fails there. Test all succeed on my end. Retrigger Ci maybe?

[rhaschke]

Looks reasonable, but testserver test fails.

[LeroyR]

Looks reasonable, but testserver test fails.

Yeah i just dont get why 😅
I can run the test just fine but i am based on melodic.

[LeroyR]

This really fails on noetic but upstream also fails the tests.
Runs fine on melodic 😄

Broken upsteam:

FROM ros:noetic-ros-base

RUN apt-get -qq update 
RUN apt-get -qq install -y wget git sudo xvfb mesa-utils ccache ssh 
RUN apt-get -qq install -y ros-noetic-tf ros-noetic-catkin ros-noetic-geometry2

RUN git clone --branch noetic-devel https://github.com/ros-visualization/interactive_markers.git 
RUN mkdir -p interactive_markers/build
CMD cd interactive_markers/build && . /opt/ros/noetic/setup.sh && cmake .. && make tests && make run_tests 

Tests succeed on melodic

FROM ros:melodic-ros-base

RUN apt-get -qq update 
RUN apt-get -qq install -y wget git sudo xvfb mesa-utils ccache ssh 
RUN apt-get -qq install -y ros-melodic-tf ros-melodic-catkin ros-melodic-geometry2

RUN git clone --branch noetic-devel https://github.com/LeroyR/interactive_markers.git
RUN mkdir -p interactive_markers/build
CMD cd interactive_markers/build && . /opt/ros/melodic/setup.sh && cmake .. && make tests && make run_tests

[LeroyR]

Whatever broke this (and upstream) is gone, no changes here
please merge/close 😄

[rhaschke]

While I like this cleanup in general, this is also a non-trivial interface change - potentially affecting many users. Hence, I am hesitant to merge this into a released distro.
On the other hand, this change is indeed required to allow for private namespaces. Maybe other maintainers can comment, @hidmic, @jacobperron?

[jacobperron]

I think it would be best to avoid breaking behavior within an already released distro. Also, the API becomes a bit misleading, the "topic_ns" actually becomes the node's namespace.

We are doing something similar in ROS 2:

interactive_markers/src/interactive_marker_server.cpp

Lines 63 to 64 in 27808d2

	const std::string update_topic = topic_namespace + "/update";
	const std::string feedback_topic = topic_namespace + "/feedback";

However, I think we can use private names with the ROS 2 implementation since we're passing in the node interfaces to the constructor. Having a separate topic namespace is still nice since it allows one node to create multiple interactive marker servers.

[LeroyR]

However, I think we can use private names with the ROS 2 implementation since we're passing in the node interfaces to the constructor. Having a separate topic namespace is still nice since it allows one node to create multiple interactive marker servers.

For most use cases there should be no difference between upstream and this pull request as you create multiple servers in the same way e.g.

InteractiveMarkerServer( "name1"):
InteractiveMarkerServer( "name2")

or not? Could you give me an example where the behavior is different?

The ros2 interface is looking crazy 🙈 but adding additional Constructors e.g. (nh) and (nh, topic_ns) would be fine too.

[jacobperron]

Good question. I can't think of a counter-example where the behavior is different before/after this PR (besides allowing private namespaces). So maybe this PR is okay. However, I think it's still a bit misleading to call the parameter "topic_ns" when it is actually the node handle namespace. We might consider renaming it.

[dgossow]

looks like I'm a bit late to the party.

High level comment: please add unit tests for the added functionality, and perhaps tests that demonstrate that there are no unintended effects on the API