Synchronous updates #1475
Synchronous updates #1475
Conversation
There was a problem hiding this comment.
I'm not sure this is the right way to go, because of the following reasoning:
As you discovered yourself, we cannot ensure that all custom plugins use update_nh_ and its queue.
Thus, to be on the safe side, you should modify your application in such a fashion that the global queue is handled by the main thread. If you want to handle other messages asynchronously, you should configure a separate queue and process it from your threads. This way you have full control.
| @@ -344,7 +345,7 @@ void VisualizationManager::onUpdate() | |||
| resetTime(); | |||
| } | |||
|
|
|||
| ros::spinOnce(); | |||
| private_->update_queue_.callAvailable(ros::WallDuration(0.01)); | |||
There was a problem hiding this comment.
| private_->update_queue_.callAvailable(ros::WallDuration(0.01)); | |
| private_->update_queue_.callAvailable(ros::WallDuration()); | |
| ros::getGlobalCallbackQueue()->callAvailable(ros::WallDuration()); |
| @@ -221,6 +221,13 @@ bool VisualizerApp::init( int argc, char** argv ) | |||
| ros::NodeHandle private_nh("~"); | |||
| reload_shaders_service_ = private_nh.advertiseService("reload_shaders", reloadShaders); | |||
|
|
|||
| //process any callbacks that don't use the designated callback queues of VisualizationManager | |||
There was a problem hiding this comment.
Why do you want to handle the global queue differently?
There was a problem hiding this comment.
VisualizerApp is used by the rviz binary but not by library users. Handling the global queue here will make sure that plugins that rely on it won't see any difference. For library users it allows not handling the global queue in the main thread.
|
@simonschmeisser, what is your feeling about this PR? Did you find an alternative solution? |
|
I have to admit that I haven't worked on this the last three weeks. The issue is still open and I understand your point about there potentially being too many non-conformant plugins. This approach would allow fixing the set of plugins we use while not changing anything for others. I will however also check the alternative you mentioned of using the AsyncSpinner for non-gui data handling and the global queue for GUI stuff only. |
while the documentation suggests this, getUpdateQueue returns the global callback queue which might be processed in different threads if AsyncSpinner is used or other threads call ros::spinOnce
simonschmeisser
force-pushed
the
synchronous-updates
branch
from
657f2ce
to
acd7516
Compare
|
@simonschmeisser, can we close this? |
The api documentation of
rviz::Displayclaims to provide aros::NodeHandlewhose associated callback queue is processed synchronously from the gui thread. Following this promise most subclasses of Display ignore thread-safety in their callback-functions. This promise however breaks if anAsyncSpinneris used in an application using therviz::RenderPanel(orros::spinOncegets called in different threads)This PR therefore adds a separate
CallbackQueuefor synchronous processing of message callbacks such that the promise continues to hold. Since not all Display subclasses are expected to use the designatedNodeHandles [1], a second commit adds a periodicros::spinOnceinrviz::VisualizerAppthat processes the global ros callback queue.Note that the second commit requires Qt5, I can rewrite it using a member function if desired
1: https://github.com/ros-planning/moveit/blob/e8a4072727abe4c704d009f05baa764bcf106b33/moveit_ros/visualization/robot_state_rviz_plugin/src/robot_state_display.cpp#L300