PickNikRobotics / rviz_visual_tools

C++ API wrapper for displaying shapes and meshes in Rviz
774 stars 243 forks source link

Trigger loadMarkerPub on init #37

Closed VictorLamoine closed 8 years ago

VictorLamoine commented 8 years ago

loadMarkerPub is called when publishMarkers is called. However if we want to make sure that the publisher has a subscriber before publishing; we need to call waitForMarkerPub.

But if waitForMarkerPub is called before any publishMarkers, the publisher has not been created and thus has an empty topic name, which leads to an infinite loop.

Initializing the marker in the constructor looks like the right thing to do.

VictorLamoine commented 8 years ago

I don't understand why you name this function waitForMarkerPub, it waits for a subscriber on the internal publisher.

Why not just use:

bool RvizVisualTools::waitForSubscriber(double wait_time = 0.5, bool blocking = false, const ros::Publisher &pub = pub_rviz_markers_)

That way you can call

waitForSubscriber();
waitForSubscriber(0.5);
waitForSubscriber(0.0, true);

without providing an extra publisher

davetcoleman commented 8 years ago

I wanted to keep this class as light-weight as possible at initialization - partly because moveit_visual_tools inherits from this class and it has its own additional publishers that it loads. It could be the case that no one ever publishes standard rviz markers, so I didn't want to initialize it until it was needed. Another use case is that I always initialize rviz_visual_tools in my application, even when my application is running in "headless" mode without publishing to Rviz. In this case I don't want to waste time waiting for the connection.

In the comments of waitForMarkerPub() it already says:

Optional blocking function to call after calling loadMarkerPub()

As for the naming, it seems either name works... I was thinking it meant "wait for the publisher to connect to a subscriber". I've had this package out for a couple years now so I'm hesitant to change major things like that.

VictorLamoine commented 8 years ago

Right, it makes sense to ask for the user to manually load the marker publisher before waiting for subscribers on this publisher. I did not see it that way but now I think it's right.

As for the name, it was just a suggestion, I'll get used to that name :)

davetcoleman commented 8 years ago

Perhaps you could clarify the name for yourself and others by adding to its doxygen comments?