From fa84bbe5ed675f7fe8e15c12b90ceaf888443057 Mon Sep 17 00:00:00 2001 From: Michael Orlov Date: Tue, 12 Sep 2023 08:41:36 -0700 Subject: [PATCH 1/3] Don't warn for unknown types if topics are not in the list Signed-off-by: Michael Orlov --- .../src/rosbag2_transport/topic_filter.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp b/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp index b82ee0f0fe..08495d86c2 100644 --- a/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp +++ b/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp @@ -121,11 +121,6 @@ bool TopicFilter::take_topic( return false; } - const std::string & topic_type = topic_types[0]; - if (!allow_unknown_types_ && !type_is_known(topic_name, topic_type)) { - return false; - } - if (!record_options_.include_hidden_topics && topic_is_hidden(topic_name)) { RCUTILS_LOG_WARN_ONCE_NAMED( ROSBAG2_TRANSPORT_PACKAGE_NAME, @@ -163,6 +158,11 @@ bool TopicFilter::take_topic( return false; } + const std::string & topic_type = topic_types[0]; + if (!allow_unknown_types_ && !type_is_known(topic_name, topic_type)) { + return false; + } + return true; } From 216d70e8831136026a31bffd8da3fc994c38f2f1 Mon Sep 17 00:00:00 2001 From: Michael Orlov Date: Tue, 12 Sep 2023 14:01:40 -0700 Subject: [PATCH 2/3] Add unit tests to verify regression Signed-off-by: Michael Orlov --- .../rosbag2_transport/test_topic_filter.cpp | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/rosbag2_transport/test/rosbag2_transport/test_topic_filter.cpp b/rosbag2_transport/test/rosbag2_transport/test_topic_filter.cpp index f36a1c0fbf..a4ce8c2fea 100644 --- a/rosbag2_transport/test/rosbag2_transport/test_topic_filter.cpp +++ b/rosbag2_transport/test/rosbag2_transport/test_topic_filter.cpp @@ -203,3 +203,41 @@ TEST_F(RegexFixture, regex_all_and_filter) auto filtered_topics = filter.filter_topics(topics_and_types_); EXPECT_THAT(filtered_topics, SizeIs(6)); } + +TEST_F(RegexFixture, do_not_print_warning_about_unknown_types_if_topic_is_not_selected) { + { // Check for topics explicitly selected via "topics" list + rosbag2_transport::RecordOptions record_options; + // Select only one topic with name "/planning" via topic list + record_options.topics = {"/planning"}; + record_options.all = false; + rosbag2_transport::TopicFilter filter{record_options, nullptr, false}; + testing::internal::CaptureStderr(); + auto filtered_topics = filter.filter_topics(topics_and_types_); + std::string test_output = testing::internal::GetCapturedStderr(); + ASSERT_EQ(0u, filtered_topics.size()); + EXPECT_TRUE( + test_output.find( + "Topic '/invalid_topic' has unknown type 'invalid_topic_type'") == std::string::npos); + EXPECT_TRUE( + test_output.find( + "Topic '/planning' has unknown type 'planning_topic_type'") != std::string::npos); + } + + { // Check for topics selected via regex + rosbag2_transport::RecordOptions record_options; + // Select only one topic with name "/planning" via regex + record_options.regex = "^/planning"; + record_options.all = false; + rosbag2_transport::TopicFilter filter{record_options, nullptr, false}; + testing::internal::CaptureStderr(); + auto filtered_topics = filter.filter_topics(topics_and_types_); + std::string test_output = testing::internal::GetCapturedStderr(); + ASSERT_EQ(0u, filtered_topics.size()); + EXPECT_TRUE( + test_output.find( + "Topic '/invalid_topic' has unknown type 'invalid_topic_type'") == std::string::npos); + EXPECT_TRUE( + test_output.find( + "Topic '/planning' has unknown type 'planning_topic_type'") != std::string::npos); + } +} From c00d21018074ebab4cdf3760d5ebf9dc61c1e652 Mon Sep 17 00:00:00 2001 From: Michael Orlov Date: Wed, 13 Sep 2023 09:14:04 -0700 Subject: [PATCH 3/3] Move checks for single type and hidden topics to the bottom Rationale: To avoid irrelevant warning messages if topic name hasn't been selected by topic list or regex. Signed-off-by: Michael Orlov --- .../src/rosbag2_transport/topic_filter.cpp | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp b/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp index 08495d86c2..a09f6b70c3 100644 --- a/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp +++ b/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp @@ -117,17 +117,6 @@ std::unordered_map TopicFilter::filter_topics( bool TopicFilter::take_topic( const std::string & topic_name, const std::vector & topic_types) { - if (!has_single_type(topic_name, topic_types)) { - return false; - } - - if (!record_options_.include_hidden_topics && topic_is_hidden(topic_name)) { - RCUTILS_LOG_WARN_ONCE_NAMED( - ROSBAG2_TRANSPORT_PACKAGE_NAME, - "Hidden topics are not recorded. Enable them with --include-hidden-topics"); - return false; - } - if (!record_options_.include_unpublished_topics && node_graph_ && topic_is_unpublished(topic_name, *node_graph_)) { @@ -158,6 +147,17 @@ bool TopicFilter::take_topic( return false; } + if (!has_single_type(topic_name, topic_types)) { + return false; + } + + if (!record_options_.include_hidden_topics && topic_is_hidden(topic_name)) { + RCUTILS_LOG_WARN_ONCE_NAMED( + ROSBAG2_TRANSPORT_PACKAGE_NAME, + "Hidden topics are not recorded. Enable them with --include-hidden-topics"); + return false; + } + const std::string & topic_type = topic_types[0]; if (!allow_unknown_types_ && !type_is_known(topic_name, topic_type)) { return false;