Make transformation framework pluggable #346

greimela-si · 2018-08-01T07:31:38Z

This change allows developers to provide their own transformation library to be used instead of tf2.

Two transformer plugins are bundled with RViz:

A tf2 plugin which transforms using the tf2 library
A trivial identity plugin which always uses the identity as transformation

The tf2 plugin is used as default, which means users normally do not have to interact with the transformation plugins.

We also added a new transformation panel to change the transformation plugin at runtime.
This panel displays all transformation plugins available at startup, third party plugins are loaded using the pluginlib.

The documentation was extended accordingly to support developers creating their own transformation plugin.

greimela-si · 2018-08-01T07:36:41Z

CI:

Linux
Linux-aarch64
macOS
Windows

Martin-Idel-SI · 2018-08-07T11:52:26Z

CI after rebase:

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2018-08-09T07:13:26Z

Sorry looks like I caused a merge conflict by merging other stuff. I got part of the way through the review, and I'll try to continue it soon.

Martin-Idel-SI · 2018-08-09T07:23:02Z

No worries, we'll rebase.

botteroa-si · 2018-08-09T16:02:26Z

New CI after rebase:

Linux
Linux-aarch64
macOS
Windows

wjwwood

Ok, I finally got through the review, sorry it took so long, but this was a large change, I had to break it into three sessions.

So the code review looks pretty good to me, I have some style things I want to address, but I'll do that in a pull request probably tomorrow.

Unfortunately, my testing of the new UI elements revealed some strange behavior. I made a video of me clicking around on it (the most interesting issue is towards the end):

https://media.giphy.com/media/1hMhlfukXkMhsX2Thq/giphy.mp4

The issues I see in the video:

the save and reset buttons do not disable after saving sometimes
what does the reset button do?
after using the reset button I can sometimes check both boxes...

I also got a segmentation fault once but I wasn't running in gdb and I could not reproduce it, but I was doing stuff similar to what I was doing in the video, just clicking around.

I also got this log message when clicking around:

[ERROR] []: Failed to remove time jump callback

Which seems to be from the destruction of an rclcpp::Clock class and it's time jump callback, here:

https://github.com/ros2/rclcpp/blob/a55e320e6eb3364258360d216657bec38020e0b5/rclcpp/src/rclcpp/clock.cpp#L143

Which was added in this pull request:

ros2/rclcpp#543

/cc @sloretz in case you have any idea how our use of clock in this pr might cause that behavior.

I know you guys have been waiting to get this merged for a long time (sorry, my fault) but I think the UI issues need to be investigated before we merge this.

wjwwood · 2018-09-05T22:11:58Z

docs/plugin_development.md

 To make them available at runtime, use the cmake macro `register_rviz_ogre_media_exports` defined in `rviz_rendering`
 For instance, if you want to register a folder `test_folder/scripts` which resides in your project's source folder, write
 ```
 register_rviz_ogre_media_exports(DIRECTORIES "test_folder/scripts")
 ```
 *Note:* If you want to export folder hierarchies, each folder needs to be exported separately. Folders within exported folders are not automatically available.

+#### Writing a display which can only work with one transformation plugin
+
+Some of the default displays (e.g. TF display or LaserScan display) can only work with tf2 as transformation framework.


Why can the LaserScan display only work with tf2?

The laser_geometry package used internally needs a tf2_buffer. Since it is not very easy to get around this (one would need to heavily rewrite some parts in laser_geometry), we decided that it's probably better to leave it out for now.

wjwwood · 2018-09-05T22:13:32Z

docs/plugin_development.md

+The class `rviz_default_plugins::transformation::TransformerGuard` helps you to handle these possibilities.
+
+As explained in the relative section, every transformation plugin implements the base class `rviz_common::transformation::FrameTransformer`.
+The `TransformerGuard` class is templated on the type of this implementation and will make sure that the display by which is owned will be disabled if the transformer currently in use is of a different type.


"... by which it is owned ..."?

wjwwood · 2018-09-05T22:15:50Z

docs/plugin_development.md

+The `TransformerGuard` class is templated on the type of this implementation and will make sure that the display by which is owned will be disabled if the transformer currently in use is of a different type.
+
+When writing a display that is supposed to be working only with a specific transformation plugin, you can add a templated instance of `TransformerGuard` as a display member.
+The constructor of this class takes two argument: a raw pointer to the display that owns it, and the name of the transformer it is supposed to work with (needed to correctly set the error status).


Can it work with more than one kind? Or can you only have one TransformerGuard per display?

No. Onle one TransformerGuard per display is supported.
Reason: The only reason for a display to require a special FrameTransformer would be if the FrameTransformer interface is not sufficient. I think there is likely only a single frame transformer plugin that fulfills this special requirement (at least that the display knows of).

If support for more allowed transformer plugins is really necessary it should be possible the extend TransformerGuard to support multiple template parameters for the supported plugins but still have only a single guard member in a display.

wjwwood · 2018-09-05T23:04:16Z

rviz_common/include/rviz_common/transformation/frame_transformer.hpp

+  virtual ~TransformationLibraryConnector() = default;
+};
+
+using TransformationLibraryConnectorPtr = std::weak_ptr<TransformationLibraryConnector>;


Usually the analogy for Ptr is a raw pointer or unique_ptr (because it has almost no overhead compared to raw pointer), however weak_ptr does have overhead. I'd recommend using WeakPtr as the suffix here.

Or just use std::weak_ptr<TransformationLibraryConnector> everywhere.

We will use the ::WeakPtr convention.

wjwwood · 2018-09-06T01:03:19Z

rviz_default_plugins/include/rviz_default_plugins/transformation/transformer_guard.hpp

+{
+
+/** @brief Base class for TransformerGuard, needed because Qt's moc and c++
+ * templates don't work nicely together. Not intended to be used directly.


Can you please elaborate on this point?

Same reason as with rviz_common::RosTopicdisplay and rviz_common::_RosTopicDisplay: a QObject may not be a template class due to limitations imposed by the Qt moc.

wjwwood · 2018-09-06T01:40:00Z

rviz_default_plugins/src/rviz_default_plugins/transformation/tf_frame_transformer.cpp

+  } catch (const tf2::ExtrapolationException & exception) {
+    throw rviz_common::transformation::FrameTransformerException(exception.what());
+  } catch (const tf2::InvalidArgumentException & exception) {
+    throw rviz_common::transformation::FrameTransformerException(exception.what());


So much information is lost here... Maybe you should prefix the error message with the original name of the error.

Martin-Idel-SI · 2018-09-06T14:19:28Z

Thank you for the feedback! I investigated the UI issues:

I removed the reset button. It doesn't really do much, especially since I doubt any user will ever need it.
We have still an issue where sometimes, changing the checkbox does not enable/disable the save button. As far as I can see, this is a bug in Qt: For an unknown reason, very rarely, the QtTreeWidget clicked signal is not sent or does not arrive, while the checkbox signal does.

There are two simple workarounds that I see, which would you prefer @wjwwood :

Don't disable the save button ever and make it a noop if the transformer hasn't changed
Disable the setValue method of the checkbox. This means that in the rare cases where previously the state of the checkboxes changed but the button was not disabled/enabled, nothing will happen. It'll be as if the user missed the target. Since at least on our systems this happens quite rarely (I usually clicked around for a couple of minutes to see this behaviour), I think hardly any user would ever even notice.

The latter would be a achieved by making setValue in grouped_checkbox_property.cpp a noop. Then we provide the functionality in a new method to be called by transformation_panel.cpp.

greimela-si · 2018-09-06T14:29:31Z

I also got a segmentation fault once but I wasn't running in gdb and I could not reproduce it, but I was doing stuff similar to what I was doing in the video, just clicking around.

You are right, these issues have been introduced after ros2/rclcpp#543.

We managed to track the issue inside the rcl and opened an issue there: ros2/rcl#293.
Short version: The reallocation of freed memory resulted in undefined behavior.

Martin-Idel-SI · 2018-09-06T15:10:53Z

CI:

Linux
Linux-aarch64
macOS
Windows

sloretz · 2018-09-06T16:56:21Z

I also got this log message when clicking around:

[ERROR] []: Failed to remove time jump callback
Which seems to be from the destruction of an rclcpp::Clock class and it's time jump callback, here:

https://github.com/ros2/rclcpp/blob/a55e320e6eb3364258360d216657bec38020e0b5/rclcpp/src/rclcpp/clock.cpp#L143

Which was added in this pull request:

ros2/rclcpp#543

Looking into this.

wjwwood · 2018-09-06T20:46:59Z

Thanks for the fast feedback.

There are two simple workarounds that I see, which would you prefer @wjwwood :

The second seems reasonable to me.

It looks like the Linux tests are taking a long time due to failing tests. My first instinct is to rebase this change, but I don't think much changed within rviz that might affect this pr.

I'll probably do that to keep the ball rolling.

…sformer in visual testing config

-Necessary after PR ros2#340

- use "::WeakPtr" convention - add missing visibility macro to TransformationManager - retain information about tf exceptions when re-throwing - fix documentation typo

- superfluous for small amount of checkboxes - Code-Style: reorder functions in transformer panel

wjwwood · 2018-09-06T21:44:21Z

I pushed the rebase to the branch bsinno/feature/use_pluginlib_for_frame_transformer on this repository, and I'm going to test that, then force push to this pr if it works.

wjwwood · 2018-09-06T21:48:08Z

I'm just running on Linux right now:

wjwwood · 2018-09-06T22:55:49Z

Ok that run looked good so I force pushed the rebase. I'm just checking on a few things and then I'll do CI on your branch (what this pr is actually testing).

Signed-off-by: William Woodall <william@osrfoundation.org>

wjwwood · 2018-09-06T23:51:32Z

CI:

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2018-09-07T01:16:46Z

I think the only thing blocking this right now is the resolution of what to do w.r.t. the save button's behavior. I'll be around tonight to follow up and make more CI jobs to hopefully merge it.

- Clicking on the transformers view, sometimes the clicked event is only sent to the underlying property, not the tree view - This results in weird behaviour of the gui - With this fix, it can only (rarely) happen that the click does nothing

Martin-Idel-SI · 2018-09-07T09:06:14Z

Ok, I implemented and documented the workaround. Tested it locally and it seems to work as intended. CI:

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2018-09-07T09:13:58Z

I pushed a style fixup, but it doesn't affect code, so the CI is valid. Consider it merged if the CI is green. I'll merge it in the morning.

Signed-off-by: William Woodall <william@osrfoundation.org>

wjwwood · 2018-09-08T01:48:28Z

Sorry, I had a trailing whitespace in my change, and there's some strange test failures listed on the linux job, I'm running another CI to confirm:

Linux
Linux-aarch64
macOS
Windows

esteve added the in review Waiting for review (Kanban column) label Aug 1, 2018

Martin-Idel-SI force-pushed the feature/use_pluginlib_for_frame_transformer branch from 6f6b2c2 to a312050 Compare August 7, 2018 11:51

wjwwood self-assigned this Aug 9, 2018

botteroa-si force-pushed the feature/use_pluginlib_for_frame_transformer branch from a312050 to f7447e2 Compare August 9, 2018 16:00

wjwwood reviewed Sep 6, 2018

View reviewed changes

Martin-Idel-SI and others added 15 commits September 6, 2018 14:43

Extract frame transformer from interface

c6fafad

Delete tf2 dependency from frame_manager

a4eb84e

Improve interface

cd60589

Fix uncrustify

bcae120

Add tests for interface in frame_manager

047ce24

Add frame_transform_tf test

10b3e7e

Use weak_ptr instead of shared_ptr for internal object

4aca341

Add visibility control

34f8e0c

Fix linters

0e7b56c

Delete superfluous code

25addf1

Add missing include to fix build

62c765d

Change name of FrameTransformer member of FrmeManager

a5c1d01

Refactor frame_transformer_tf_test

b38626c

Add methods to TFWrapper and make buffer_ private

9117995

Fix rviz_default_plugins after changes in TFWrapper

7549ed8

botteroa-si and others added 5 commits September 6, 2018 14:43

Update comment in transformer_guard.hpp and insert TF as default tran…

2873d35

…sformer in visual testing config

Pass clock to tf2::Buffer contructor

b27d8f6

-Necessary after PR ros2#340

Fix tests and uncrustify

9dfd0a8

Minor cleanups

5f7cdf3

- use "::WeakPtr" convention - add missing visibility macro to TransformationManager - retain information about tf exceptions when re-throwing - fix documentation typo

Remove reset button from transformer panel

356f60b

- superfluous for small amount of checkboxes - Code-Style: reorder functions in transformer panel

wjwwood force-pushed the feature/use_pluginlib_for_frame_transformer branch from 3216dd5 to 356f60b Compare September 6, 2018 22:54

wjwwood added 4 commits September 6, 2018 16:46

style and whitespace fixes

c389501

Signed-off-by: William Woodall <william@osrfoundation.org>

use ++ suffix in loop iterators (cppcheck)

fe7db36

Signed-off-by: William Woodall <william@osrfoundation.org>

suppress Ogre warnings when interpreting single header (autocomplete)

17ba6b4

Signed-off-by: William Woodall <william@osrfoundation.org>

initialize member variable in constructor

a04e711

Signed-off-by: William Woodall <william@osrfoundation.org>

wjwwood approved these changes Sep 7, 2018

View reviewed changes

doc style cleanup

7d644ff

wjwwood approved these changes Sep 7, 2018

View reviewed changes

fixup

8898a67

Signed-off-by: William Woodall <william@osrfoundation.org>

wjwwood merged commit 48c900d into ros2:ros2 Sep 8, 2018

wjwwood removed the in review Waiting for review (Kanban column) label Sep 8, 2018

Martin-Idel-SI deleted the feature/use_pluginlib_for_frame_transformer branch September 10, 2018 06:45

Martin-Idel-SI mentioned this pull request Sep 10, 2018

[rviz_common] cleanup usage of tf2 buffer versus transform listener #52

Closed

andreasholzner mentioned this pull request Feb 8, 2019

Re-enable use of tf message filter #375

Merged

ymd-stella mentioned this pull request May 29, 2020

High latency in TF propagation #359

Closed

sloretz mentioned this pull request Aug 31, 2022

Make Axes display use latest transform #892

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make transformation framework pluggable #346

Make transformation framework pluggable #346

greimela-si commented Aug 1, 2018

greimela-si commented Aug 1, 2018

Martin-Idel-SI commented Aug 7, 2018

wjwwood commented Aug 9, 2018

Martin-Idel-SI commented Aug 9, 2018

botteroa-si commented Aug 9, 2018

wjwwood left a comment

wjwwood Sep 5, 2018

Martin-Idel-SI Sep 6, 2018

wjwwood Sep 5, 2018

wjwwood Sep 5, 2018

anhosi Sep 6, 2018

wjwwood Sep 5, 2018

wjwwood Sep 5, 2018

anhosi Sep 6, 2018

wjwwood Sep 6, 2018

anhosi Sep 6, 2018 •

edited

wjwwood Sep 6, 2018

Martin-Idel-SI commented Sep 6, 2018 •

edited

greimela-si commented Sep 6, 2018

Martin-Idel-SI commented Sep 6, 2018 •

edited

sloretz commented Sep 6, 2018

wjwwood commented Sep 6, 2018

wjwwood commented Sep 6, 2018

wjwwood commented Sep 6, 2018

wjwwood commented Sep 6, 2018

wjwwood commented Sep 6, 2018

wjwwood commented Sep 7, 2018

Martin-Idel-SI commented Sep 7, 2018 •

edited

wjwwood commented Sep 7, 2018

wjwwood commented Sep 8, 2018

Make transformation framework pluggable #346

Make transformation framework pluggable #346

Conversation

greimela-si commented Aug 1, 2018

greimela-si commented Aug 1, 2018

Martin-Idel-SI commented Aug 7, 2018

wjwwood commented Aug 9, 2018

Martin-Idel-SI commented Aug 9, 2018

botteroa-si commented Aug 9, 2018

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anhosi Sep 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martin-Idel-SI commented Sep 6, 2018 • edited

greimela-si commented Sep 6, 2018

Martin-Idel-SI commented Sep 6, 2018 • edited

sloretz commented Sep 6, 2018

wjwwood commented Sep 6, 2018

wjwwood commented Sep 6, 2018

wjwwood commented Sep 6, 2018

wjwwood commented Sep 6, 2018

wjwwood commented Sep 6, 2018

wjwwood commented Sep 7, 2018

Martin-Idel-SI commented Sep 7, 2018 • edited

wjwwood commented Sep 7, 2018

wjwwood commented Sep 8, 2018

anhosi Sep 6, 2018 •

edited

Martin-Idel-SI commented Sep 6, 2018 •

edited

Martin-Idel-SI commented Sep 6, 2018 •

edited

Martin-Idel-SI commented Sep 7, 2018 •

edited