Closed zmichaels11 closed 4 years ago
While the code generally LGTM, how could we cope with
get_storage_identifier()
andget_bagfile_size()
being unimplemented here before? Are we in the process of expending the API?
Yeah, both functions were added into the API in anticipation of supporting bagfile splitting. I'll link the merged PR numbers related to these changes.
:+1: Thanks!
does it make sense to enhance this PR to comply with https://github.com/ros2/rosbag2/pull/184?
does it make sense to enhance this PR to comply with ros2/rosbag2#184?
Yes, unless you want to merge this commit first.
sounds good. Please go ahead and advance this PR with the changes needed and I we can run CI together on this.
FYI, I've tried to use the current state of this PR and get the following compilation error:
In file included from /home/jacob/ws/ros1_bridge_ws/install/class_loader/include/class_loader/class_loader_core.hpp:56:0,
from /home/jacob/ws/ros1_bridge_ws/install/class_loader/include/class_loader/class_loader.hpp:55,
from /home/jacob/ws/ros1_bridge_ws/install/class_loader/include/class_loader/multi_library_class_loader.hpp:52,
from /home/jacob/ws/ros1_bridge_ws/install/pluginlib/include/pluginlib/class_loader.hpp:58,
from /home/jacob/ws/rosbag2_bag_v2_ws/install/include/rosbag/bag.h:67,
from /home/jacob/ws/rosbag2_bag_v2_ws/src/rosbag2_bag_v2/rosbag2_bag_v2_plugins/src/rosbag2_bag_v2_plugins/storage/rosbag_v2_storage.hpp:23,
from /home/jacob/ws/rosbag2_bag_v2_ws/src/rosbag2_bag_v2/rosbag2_bag_v2_plugins/src/rosbag2_bag_v2_plugins/storage/rosbag_v2_storage.cpp:15:
/home/jacob/ws/ros1_bridge_ws/install/class_loader/include/class_loader/meta_object.hpp: In instantiation of ‘B* class_loader::impl::MetaObject<C, B>::create() const [with C = rosbag2_bag_v2_plugins::RosbagV2Storage; B = rosbag2_storage::storage_interfaces::ReadOnlyInterface]’:
/home/jacob/ws/rosbag2_bag_v2_ws/src/rosbag2_bag_v2/rosbag2_bag_v2_plugins/src/rosbag2_bag_v2_plugins/storage/rosbag_v2_storage.cpp:185:1: required from here
/home/jacob/ws/ros1_bridge_ws/install/class_loader/include/class_loader/meta_object.hpp:193:12: error: invalid new-expression of abstract class type ‘rosbag2_bag_v2_plugins::RosbagV2Storage’
return new C;
^~~~~
In file included from /home/jacob/ws/rosbag2_bag_v2_ws/src/rosbag2_bag_v2/rosbag2_bag_v2_plugins/src/rosbag2_bag_v2_plugins/storage/rosbag_v2_storage.cpp:15:0:
/home/jacob/ws/rosbag2_bag_v2_ws/src/rosbag2_bag_v2/rosbag2_bag_v2_plugins/src/rosbag2_bag_v2_plugins/storage/rosbag_v2_storage.hpp:29:7: note: because the following virtual functions are pure within ‘rosbag2_bag_v2_plugins::RosbagV2Storage’:
class RosbagV2Storage : public rosbag2_storage::storage_interfaces::ReadOnlyInterface
^~~~~~~~~~~~~~~
In file included from /home/jacob/ws/ros1_bridge_ws/install/rosbag2_storage/include/rosbag2_storage/storage_interfaces/read_only_interface.hpp:20:0,
from /home/jacob/ws/rosbag2_bag_v2_ws/src/rosbag2_bag_v2/rosbag2_bag_v2_plugins/src/rosbag2_bag_v2_plugins/storage/rosbag_v2_storage.hpp:22,
from /home/jacob/ws/rosbag2_bag_v2_ws/src/rosbag2_bag_v2/rosbag2_bag_v2_plugins/src/rosbag2_bag_v2_plugins/storage/rosbag_v2_storage.cpp:15:
/home/jacob/ws/ros1_bridge_ws/install/rosbag2_storage/include/rosbag2_storage/storage_interfaces/base_info_interface.hpp:41:23: note: virtual std::__cxx11::string rosbag2_storage::storage_interfaces::BaseInfoInterface::get_relative_path() const
virtual std::string get_relative_path() const = 0;
^~~~~~~~~~~~~~~~~
make[2]: *** [CMakeFiles/rosbag2_bag_v2_plugins.dir/src/rosbag2_bag_v2_plugins/storage/rosbag_v2_storage.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [CMakeFiles/rosbag2_bag_v2_plugins.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [all] Error 2
@jacobperron just resolved that issue; it was due to recent API changes introduced in ros2/rosbag2#184.
@Karsten1987 This PR is ready
@zmichaels11 Yep, thanks. I'm able to compile now.
@Karsten1987 is there anything blocking this PR from merging?
Implemented changes in IO interfaces that were introduced in recent commits to rosbag2.
Dependent PRs