Use dedicated TransformListener thread #551
Conversation
|
+1. This seems to make everything a lot smoother in RVIZ2 (for instance, when a robot arm is moving, it's very jerky without this patch). |
| *buffer_, rviz_ros_node.lock()->get_raw_node(), false); | ||
| if (using_dedicated_thread) { | ||
| tf_listener_ = std::make_shared<tf2_ros::TransformListener>( | ||
| *buffer_, true); |
There was a problem hiding this comment.
This will actually create a new node, did you try what happens if you just use std::make_shared<tf2_ros::TransformListener>( *buffer_, rviz_ros_node.lock()->get_raw_node(), true); - shouldn't this already spin the TransformListener in a dedicated thread?
There was a problem hiding this comment.
Node has already been added to an executor. was shown.
relevant code:
There was a problem hiding this comment.
Okay, I see... So the correct way to solve this should be to actually make RViz multi-threaded and spin the transformer callback in its own thread then (and more often than e.g. the update loop).
I guess this is a much more subtle undertaking, so I would propose:
- Add a "todo" comment that this actually introduces a new node and should be done via threading,
- Merge the PR since this fixes a big issue where there are multiple reports
- Open an issue (or use this one: ROS abstraction needs a (multithreaded) executor to add nodes to #548) to track the todo
There was a problem hiding this comment.
Add a "todo" comment that this actually introduces a new node and should be done via threading,
If you provide the concrete comment, I'll add them to this PR. (I don't know the format of the TODO comments on this project.)
There was a problem hiding this comment.
No real schema except for linters. The following should do (you can also add your name, but I find the naming inconsistent, it's just enforced by cpplint)
TODO(anyone): The TransformListener needs very quick spinning so it uses its own node here. Remove this in favor of a multithreaded spinner and ensure that the listener callback queue does not fill up.
You'll probably need to split it up into multiple lines.
@jacobperron The error analysis seems sound and the solution works on several machines. When this got ported we explicitly avoided making new nodes if possible, but making RViz multithreaded requires much more work, since this is an issue many people complained about, I think this is a good compromise - unless @ymd-stella wants to jump in and do this of course.
Maybe you could take a quick look?
There was a problem hiding this comment.
I agree that a multithreaded executor sounds like the best way to go. I'm okay with merging this workaround in the meantime with a TODO as you suggest.
There was a problem hiding this comment.
I've added the comment. I've written TODO(pull/551) to make it easier for those who found it to look up the details.
ymd-stella
force-pushed
the
use-dedicated-tf-listener-thread
branch
from
bd03113
to
f0442de
Compare
Signed-off-by: ymd-stella <world.applepie@gmail.com>
ymd-stella
force-pushed
the
use-dedicated-tf-listener-thread
branch
from
f0442de
to
f528bfd
Compare
|
The Rpr_* job is failing for an unrelated reason.
|
There was a problem hiding this comment.
LGTM
@ymd-stella Thanks for the investigation and the patch!
* restore compatibility with older Qt versions (#561) Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com> * Suppress warnings when building with older Qt versions. (#562) Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Don't try to moc generate env_config.hpp file. (#550) This removes one more warning from rviz_common builds. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * rewrite hack to avoid CMake warning with assimp 5.0.1 and older, apply cross platform (#565) Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com> * Use dedicated TransformListener thread (#551) Signed-off-by: ymd-stella <world.applepie@gmail.com> * restore alphabetical include order (#563) Signed-off-by: Karsten Knese <karsten@openrobotics.org> * Don't install test header files in rviz_rendering. (#564) * Don't install test header files in rviz_rendering. This change started as a relatively simple try at not installing test includes when installing rviz_rendering. As you can see, it ballooned into quite a large change, so here is an explanation of why. We shouldn't install test include header files when installing the package; that just ends up on the end user system, and is a non-supported API. Worse, we don't want to compile test code in our main library. Yet rviz_rendering is currently doing both of these things. Unfortunately, it is somewhat tricky to make that code and header file private. The problem is that that test code is used in rviz_rendering, rviz_common, rviz_default_plugins, and rviz_rendering_tests. One solution might be to extract that test code into its own package, and then have all of the other packages depend on it. The problem is that that test package would both be depended on by rviz_rendering (for tests), and depends itself on rviz_rendering (for its functionality), creating a dependency loop. The solution I opted for here is to copy the test code into the appropriate packages. This leads to some duplication of functionality, but it effectively breaks the dependency loop and succeeds in eliminating the test code from our installed library. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> Co-authored-by: Dirk Thomas <dirk-thomas@users.noreply.github.com> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org> Co-authored-by: ymd-stella <7959916+ymd-stella@users.noreply.github.com> Co-authored-by: Karsten Knese <Karsten1987@users.noreply.github.com>
Signed-off-by: ymd-stella <world.applepie@gmail.com>
Signed-off-by: ymd-stella <world.applepie@gmail.com>
Fixes #359.
Problem detail is #359 (comment).