PickNikRobotics / rviz_visual_tools

C++ API wrapper for displaying shapes and meshes in Rviz
765 stars 242 forks source link

spin independent thread for RemoteControl #224

Closed v4hn closed 2 years ago

v4hn commented 2 years ago

The RemoteControl class currently spins the global queue when waiting for the user to confirm. It does so because the node handle for the subscriber that receives the feedback is usually initialized with the global callback queue. But that also means that other callbacks (including the one currently running!) can be processed in the thread that is waiting for user input and it's easy to block all available spinners or get into recursive spin calls for the same topic.

Even worse, there is no workaround: Even if the user provides a node handle with a different callback queue, the calls to spinOnce will not even process that queue but the method calls still invoke unwanted side-effects without serving any purpose.

Thus I modified the class to create its own spinner unconditionally.

@jspricke @tylerjw

codecov[bot] commented 2 years ago

Codecov Report

Merging #224 (364460a) into master (9b10fd2) will decrease coverage by 0.02%. The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #224      +/-   ##
==========================================
- Coverage   19.83%   19.81%   -0.03%     
==========================================
  Files           5        5              
  Lines        1699     1701       +2     
==========================================
  Hits          337      337              
- Misses       1362     1364       +2     
Impacted Files Coverage Δ
src/remote_control.cpp 0.00% <0.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

v4hn commented 2 years ago

~~Added a commit to imitate @jspricke's approach in #193 which does not rely on a new thread. There's really no need for a whole thread for this (even though it doesn't harm a lot either).~~

Edit: Removed that commit again. As we also want to stop autonomous mode we actually need the spinner here.