Using existing storage_default_plugins as base_class_type for other plugins about rosbag2 HOT 6 CLOSED

The base_class_type is the base class type as used by the pluginlib factory to load the plugin, not the class it is derived from. Hence it has to be rosbag2_storage_plugins::ReadWriteInterface or rosbag2_storage_plugins::ReadOnlyInterface because that are the only classes known to rosbag2_storage which can be loaded dynamically. Similarly, this is the correct class to mention when using the PLUGINLIB_EXPORT_CLASS macro.

It should be no problem to derive the class from rosbag2_storage_plugins::SqliteStorage nevertheless. See for instance the usage in rviz:
The class PointStampedDisplay derives from RosTopicDisplay (which ultimately derives from rviz_common::Display):
https://github.com/ros2/rviz/blob/a8eea3365c743afb8f38470b9697a58484c2c764/rviz_default_plugins/include/rviz_default_plugins/displays/point/point_stamped_display.hpp#L77
but the base class in the pluginlib description and the export macro is the rviz_common::Display class
https://github.com/ros2/rviz/blob/a8eea3365c743afb8f38470b9697a58484c2c764/rviz_default_plugins/src/rviz_default_plugins/displays/point/point_stamped_display.cpp#L165
and
https://github.com/ros2/rviz/blob/a8eea3365c743afb8f38470b9697a58484c2c764/rviz_default_plugins/plugins_description.xml#L248

from rosbag2.

Thanks @Martin-Idel , this cleared up my confusion. I tried what you laid out and now inheriting works great. A follow up though is that, what is the significance in the naming of this CMake instruction:

Should downstream plugins seek to use different unique ..._DLL strings, or target the same ROSBAG2_STORAGE_DEFAULT_PLUGINS_BUILDING_DLL?

Although I can now inherit from rosbag2_storage_plugins::SqliteStorage, it seem little gain in that I've just noticed most of the function's I'd like to overload are not virtual, or the class variables the manipulate are private. I could rework upstream SqliteStorage to be a bit more adaptable for this, but could use some suggested strategies.

from rosbag2.

Visibility - the ROSBAG2_STORAGE_DEFAULT_PLUGINS_BUILDING_DLL switch

The comment basically explains what it does - it modifies some macros causing some compiler specific code to switch, namely dllexport rather than dllimport.

The underlying idea is called "symbol visibility". Windows DLL's have this concept that a DLL has to declare which symbols get exported. Only those symbols can then be used by a client library: Every symbol you want to use (for instance a function) in code which links against a library needs to be explicitly exported by that library calling __declspec( dllexport ). This tells the compiler "Hey, I'm implementing this symbol and I want it to be available to my clients". If you import the corresponding header in your client library, this header may then not contain __declspec( dllexport ), because the compiler would try to find the corresponding implementation, which you haven't defined. If you link against data symbols, you actually have to import the corresponding symbols explicitly using __declspec( dllimport ) and it helps the compiler if you always import symbols from other libraries. So in other words: If you define a symbol in a library which you want your clients to use, you have to dllexport it, if you declare a symbol from a different library that you want to use, you should dllimport it.

That means that you need a way to switch between __declspec( dllexport ) when building the library and __declspec( dllimport ) when including the header somewhere where it links against the library. That's what this switch does.

In other words: If you want to build a library, which you want to extend, you need to define your own macro. You can't use the one from rosbag2_storage_default_plugins, because then you can't link against rosbag2_storage_default_plugins anymore, because this piece of code will switch all rosbag2_storage_default_plugins dllimport symbols to dllexport. If your plugin should not be extended, you don't need that macro and can just do without using exports and imports.

On Unix based systems, this macro currently does nothing. However, it could do something similar, because ELF also supports symbol visibility. However, it usually has to be explicitly switched on and that's not happening for now.

Inheriting from rosbag2_storage_plugins

In general, I agree that it's not a very good idea to inherit from SqliteStorage. To be honest, I believe that the functions will change quite a lot over time. They aren't optimized for speed (and will have to be at some point) and a lot of functionality might still change. So your plugin might break in the future.

If you think there is functionality which is not really specific to the SQLite backend and which could be shared among different plugins, I'd suggest refactoring those bits of code outside of the plugin into a different class or as free functions into rosbag2_storage and use those in the SQLite code. The idea of the design is to have all functionality and interfaces which can be shared among storage plugins within rosbag2_storage and only link against that package. This way, you can freely modify any plugin without breaking somebody else's code.

from rosbag2.

@ruffsl Are you currently still working related to this issue or what's the status of this?

from rosbag2.

Revisiting this given the recent improvement announcements:

https://discourse.ros.org/t/fast-forward-merging-rosbag2-master-api-to-foxy/18927

Has anything majorly changed with respect to inheriting from rosbag2_storage_plugins?

from rosbag2.

I'm going to close this as out-of-date. A new issue could be reopened if something specific is needed to be implemented. As discovered in this discussion, there is no specific barrier to subclassing a storage plugin to create new derived types, however the plugin to be extended will need to provide suitable virtual interfaces.

I'm currently in the process of splitting out the sqlite3 plugin into its own specific package rosbag2_storage_sqlite3 rather than continuing to keep it under a package-title/heading of default_plugins.

from rosbag2.