-
Notifications
You must be signed in to change notification settings - Fork 260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed playing if unknown message types exist #592
Fixed playing if unknown message types exist #592
Conversation
1. play a specific known message type even if some unknown types exist. 2. add a warning message while a message type library not exist. Signed-off-by: Chen Lihui <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM - i am convinced by the argument that it is simpler just to warn and play anyways. Couple of minor nitpicks unrelated to the functionality - address if you like.
@@ -44,7 +44,7 @@ class MockSequentialReader : public rosbag2_cpp::reader_interfaces::BaseReaderIn | |||
|
|||
while (num_read_ < messages_.size()) { | |||
for (const auto & filter_topic : filter_.topics) { | |||
if (!messages_[num_read_ + 1]->topic_name.compare(filter_topic)) { | |||
if (!messages_[num_read_]->topic_name.compare(filter_topic)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change actually necessary?
topic.name, rosbag2_transport_->create_generic_publisher( | ||
topic.name, topic.type, topic_qos))); | ||
} catch (const std::runtime_error & e) { | ||
// using a warning log seems better than adding a new option |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment doesn't make much sense without the context of the issue conversation - i think that the text in the warning explains it well enough.
Gist: https://gist.githubusercontent.com/emersonknapp/14502914c97ff87953246acb31fd7622/raw/5a1b1739bf80b50809975253103a1e68a56ef542/ros2.repos |
The windows build:
Considering the action CI here ran the linters, let's not be blocked on that. |
1. play a specific known message type even if some unknown types exist. 2. add a warning message while a message type library not exist. Signed-off-by: Chen Lihui <[email protected]>
1. play a specific known message type even if some unknown types exist. 2. add a warning message while a message type library not exist. Signed-off-by: Chen Lihui <[email protected]>
1. play a specific known message type even if some unknown types exist. 2. add a warning message while a message type library not exist. Signed-off-by: Chen Lihui <[email protected]>
@emersonknapp I believe we should backport this to Foxy. |
1. play a specific known message type even if some unknown types exist. 2. add a warning message while a message type library not exist. Signed-off-by: Chen Lihui <[email protected]>
1. play a specific known message type even if some unknown types exist. 2. add a warning message while a message type library not exist. Signed-off-by: Chen Lihui <[email protected]> Signed-off-by: Emerson Knapp <[email protected]>
1. play a specific known message type even if some unknown types exist. 2. add a warning message while a message type library not exist. Signed-off-by: Chen Lihui <[email protected]>
1. play a specific known message type even if some unknown types exist. 2. add a warning message while a message type library not exist. Signed-off-by: Chen Lihui <[email protected]>
#592) (#686) 1. play a specific known message type even if some unknown types exist. 2. add a warning message while a message type library not exist. Signed-off-by: Chen Lihui <[email protected]>
Fixes #591
Signed-off-by: Chen Lihui [email protected]