From 95b51e7393e2613d184ad16e93ac5d4cfdf2a727 Mon Sep 17 00:00:00 2001 From: Michael Orlov Date: Sat, 18 Dec 2021 00:28:57 -0800 Subject: [PATCH] Fix logic for regular expressions in topic_filter Signed-off-by: Michael Orlov --- .../src/rosbag2_transport/topic_filter.cpp | 45 +++++++++++-------- .../src/rosbag2_transport/topic_filter.hpp | 5 ++- .../rosbag2_transport/test_topic_filter.cpp | 16 ++++--- 3 files changed, 41 insertions(+), 25 deletions(-) diff --git a/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp b/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp index e036ba9920..17677d1072 100644 --- a/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp +++ b/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp @@ -75,7 +75,7 @@ is_leaf_topic( const std::string & topic_name, rclcpp::node_interfaces::NodeGraphInterface & node_graph) { auto subscriptions_info = node_graph.get_subscriptions_info_by_topic(topic_name); - return subscriptions_info.size() == 0; + return subscriptions_info.empty(); } } // namespace @@ -83,16 +83,19 @@ namespace rosbag2_transport { TopicFilter::TopicFilter( - RecordOptions record_options, + const RecordOptions & record_options, rclcpp::node_interfaces::NodeGraphInterface::SharedPtr node_graph, bool allow_unknown_types) : record_options_(record_options), allow_unknown_types_(allow_unknown_types), node_graph_(node_graph) -{} - -TopicFilter::~TopicFilter() -{} +{ + if (record_options_.ignore_leaf_topics && !node_graph_) { + ROSBAG2_TRANSPORT_LOG_WARN_STREAM( + "Requested to filter leaf topics, however node_graph not provided. \n" + "Leaf topics will not be filtered."); + } +} std::unordered_map TopicFilter::filter_topics( const std::map> & topic_names_and_types) @@ -130,22 +133,28 @@ bool TopicFilter::take_topic( return false; } - if (!record_options_.topics.empty() && !topic_in_list(topic_name, record_options_.topics)) { - return false; - } - std::regex exclude_regex(record_options_.exclude); - if (!record_options_.exclude.empty() && std::regex_search(topic_name, exclude_regex)) { + if (!record_options_.exclude.empty() && + std::regex_search(topic_name, exclude_regex)) + { return false; } - std::regex include_regex(record_options_.regex); - if ( - !record_options_.all && // All takes precedence over regex - !record_options_.regex.empty() && // empty regex matches nothing, but should be ignored - !std::regex_search(topic_name, include_regex)) - { - return false; + if (!record_options_.all) { // All takes precedence over regex and topic list + bool topic_name_in_topic_list = + (!record_options_.topics.empty() && topic_in_list(topic_name, record_options_.topics)); + + if (!record_options_.regex.empty()) { + std::regex include_regex(record_options_.regex); + // Check if topic_name not in include_regex nor in topic list + if (!std::regex_search(topic_name, include_regex) && !topic_name_in_topic_list) { + return false; + } + } else { // In case if regex empty, check if topic name not in topic list + if (!topic_name_in_topic_list) { + return false; + } + } } return true; diff --git a/rosbag2_transport/src/rosbag2_transport/topic_filter.hpp b/rosbag2_transport/src/rosbag2_transport/topic_filter.hpp index 4335fbcd93..a9e0995d83 100644 --- a/rosbag2_transport/src/rosbag2_transport/topic_filter.hpp +++ b/rosbag2_transport/src/rosbag2_transport/topic_filter.hpp @@ -39,10 +39,11 @@ class ROSBAG2_TRANSPORT_PUBLIC TopicFilter { public: explicit TopicFilter( - RecordOptions record_options, + const RecordOptions & record_options, rclcpp::node_interfaces::NodeGraphInterface::SharedPtr node_graph = nullptr, bool allow_unknown_types = false); - virtual ~TopicFilter(); + + virtual ~TopicFilter() = default; /// Filter all topic_names_and_types via take_topic method, return the resulting filtered set /// Filtering order is: diff --git a/rosbag2_transport/test/rosbag2_transport/test_topic_filter.cpp b/rosbag2_transport/test/rosbag2_transport/test_topic_filter.cpp index 7e5488d1c5..e1f7189c41 100644 --- a/rosbag2_transport/test/rosbag2_transport/test_topic_filter.cpp +++ b/rosbag2_transport/test/rosbag2_transport/test_topic_filter.cpp @@ -49,15 +49,15 @@ TEST(TestTopicFilter, filter_hidden_topics) { {"_/topic/c", {"type_c"}}, }; + rosbag2_transport::RecordOptions record_options; + record_options.all = true; { - rosbag2_transport::RecordOptions record_options; record_options.include_hidden_topics = true; rosbag2_transport::TopicFilter filter{record_options, nullptr, true}; auto filtered_topics = filter.filter_topics(topics_and_types); ASSERT_EQ(topics_and_types.size(), filtered_topics.size()); } { - rosbag2_transport::RecordOptions record_options; record_options.include_hidden_topics = false; rosbag2_transport::TopicFilter filter{record_options, nullptr, true}; auto filtered_topics = filter.filter_topics(topics_and_types); @@ -73,7 +73,9 @@ TEST(TestTopicFilter, filter_topics_with_more_than_one_type) { {"topic/d", {"type_d", "type_d", "type_d2"}}, }; - rosbag2_transport::TopicFilter filter{rosbag2_transport::RecordOptions{}, nullptr, true}; + rosbag2_transport::RecordOptions record_options; + record_options.all = true; + rosbag2_transport::TopicFilter filter{record_options, nullptr, true}; auto filtered_topics = filter.filter_topics(topics_and_types); EXPECT_THAT(filtered_topics, SizeIs(2)); for (const auto & topic : @@ -90,7 +92,9 @@ TEST(TestTopicFilter, filter_topics_with_known_type_invalid) { {"topic/c", {"type_c"}} }; - rosbag2_transport::TopicFilter filter{rosbag2_transport::RecordOptions{}, nullptr}; + rosbag2_transport::RecordOptions record_options; + record_options.all = true; + rosbag2_transport::TopicFilter filter{record_options, nullptr}; auto filtered_topics = filter.filter_topics(topics_and_types); ASSERT_EQ(0u, filtered_topics.size()); } @@ -101,7 +105,9 @@ TEST(TestTopicFilter, filter_topics_with_known_type_valid) { {"topic/b", {"test_msgs/BasicTypes"}}, {"topic/c", {"test_msgs/BasicTypes"}} }; - rosbag2_transport::TopicFilter filter{rosbag2_transport::RecordOptions{}, nullptr}; + rosbag2_transport::RecordOptions record_options; + record_options.all = true; + rosbag2_transport::TopicFilter filter{record_options, nullptr}; auto filtered_topics = filter.filter_topics(topics_and_types); ASSERT_EQ(3u, filtered_topics.size()); }