Support specifying patterns for matching topics at run-time #34
Conversation
qsc-panvlantis
changed the title
qsc-panvlantis
force-pushed
the
topic_pattern_support
branch
from
15bdca5 to
5a4b3b7
Compare
qsc-panvlantis
force-pushed
the
topic_pattern_support
branch
from
ba5ded1 to
c13dd83
Compare
qsc-panvlantis
force-pushed
the
topic_pattern_support
branch
from
c13dd83 to
a7ed8fa
Compare
|
Thanks for working on this. Let me know when you want it to be in "review-ready" state. |
|
Hello @DLu! Thank you for your reply and I apologize once again for opening this PR early. I believe it is in ready for review now. |
|
|
||
| catkin_package( | ||
| CATKIN_DEPENDS rosbag rosbag_snapshot_msgs roscpp rosgraph_msgs std_srvs topic_tools | ||
| CATKIN_DEPENDS ${PACKAGE_DEPENDENCIES} |
There was a problem hiding this comment.
Other than adding the regex component to Boost, are any of these CMakeLists changes needed?
| bool matches(const std::string &topic) const; | ||
|
|
||
| private: | ||
| // Maximum difference in time from newest and oldest message in buffer before older messages are removed |
There was a problem hiding this comment.
| // Maximum difference in time from newest and oldest message in buffer before older messages are removed |
| @@ -110,6 +134,12 @@ struct ROSBAG_DECL SnapshotterOptions | |||
| // Provides list of topics to snapshot and their limit configurations | |||
| topics_t topics_; | |||
|
|
|||
| typedef std::vector<SnapshotterTopicPatternConstPtr> patterns_t; | |||
There was a problem hiding this comment.
Can we eliminate this typedef since it is only used the once?
| @@ -62,6 +62,11 @@ const ros::Duration SnapshotterTopicOptions::INHERIT_DURATION_LIMIT = ros::Durat | |||
| const int32_t SnapshotterTopicOptions::INHERIT_MEMORY_LIMIT = 0; | |||
| const int32_t SnapshotterTopicOptions::INHERIT_COUNT_LIMIT = 0; | |||
|
|
|||
| static bool is_topic_name_pattern(const std::string& s) | |||
| { | |||
| return s.find_first_of(".*+?()[]{}\\") != std::string::npos; | |||
There was a problem hiding this comment.
Please add to the README with information about the pattern matching syntax. This is most important so that it doesn't get mixed up with glob syntax
| if (it != matching_patterns_cache_.end()) | ||
| return it->second; | ||
| SnapshotterTopicPatternConstPtr matching_pattern = nullptr; | ||
| for (auto& pattern : patterns_) |
There was a problem hiding this comment.
Can you please put in curly braces for the for loop? Its not a strict requirement, I just dislike the look when its multiple lines (even though its one statement)
| @@ -606,10 +686,9 @@ int Snapshotter::run() | |||
| status_timer_ = nh_.createTimer(options_.status_period_, &Snapshotter::publishStatus, this); | |||
|
|
|||
| // Start timer to poll ROS master for topics | |||
| if (options_.all_topics_) | |||
There was a problem hiding this comment.
How would you feel about changing this to if (options_.all_topics_ || !patterns_.empty())
| @@ -562,6 +634,14 @@ void Snapshotter::pollTopics(ros::TimerEvent const& e, rosbag_snapshot::Snapshot | |||
| for (ros::master::TopicInfo const& t : topics) | |||
| { | |||
| std::string topic = t.name; | |||
| SnapshotterTopicOptions topic_options; | |||
| if (!options->all_topics_) | |||
There was a problem hiding this comment.
Can you add a comment here to the effect of if we are not recording all topics, we have to check to see if this topic matches any of the patterns
Hello!
This branch is supposed to provide support for regular expressions in topic specs, enabling dynamic matching of topics at run-time.
Actually,
this is a work-in-progress at the moment andthis PR was created by mistake (it was supposed to target the fork's main branch 😅) so it may be better to close it for now, but I thought it may be good to provide some context first.