Added regex filter field for TF display #1032
Conversation
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
rviz_common/include/rviz_common/properties/regex_filter_property.hpp
Outdated
Show resolved
Hide resolved
| try { | ||
| regex_.assign(value.toLocal8Bit().constData(), std::regex_constants::optimize); | ||
| } catch (const std::regex_error & e) { | ||
| regex_ = default_; | ||
| } |
There was a problem hiding this comment.
I'm not opposed to this code as-is, but I'm just wondering; can this exception actually happen given the RegexValidator we have installed?
rviz_default_plugins/include/rviz_default_plugins/displays/tf/tf_display.hpp
Outdated
Show resolved
Hide resolved
rviz_default_plugins/src/rviz_default_plugins/displays/tf/tf_display.cpp
Outdated
Show resolved
Hide resolved
| while (it != end) { | ||
| if (it->empty() || !std::regex_search(*it, filter_whitelist_property_->regex()) || | ||
| std::regex_search(*it, filter_blacklist_property_->regex())) | ||
| { | ||
| std::swap(*it, *--end); // swap current to-be-dropped name with last one | ||
| } else { | ||
| ++it; | ||
| } | ||
| } | ||
|
|
||
| std::sort(frames.begin(), end); | ||
|
|
||
| S_FrameInfo current_frames; | ||
| for (it = frames.begin(); it != end; ++it) { | ||
| FrameInfo * info = getFrameInfo(*it); | ||
| if (!info) { | ||
| info = createFrame(*it); | ||
| } else { | ||
| updateFrame(info); | ||
| } | ||
|
|
||
| current_frames.insert(info); | ||
| } |
There was a problem hiding this comment.
It feels to me like these loops are going to be doing an unnecessary amount of work, in both the case where filtering is used and where it is not used. I'm particularly worried about large TF trees.
For the first loop, if someone is not using the filtering, we are still going to be iterating over every single frame to just ignore it. It seems like if both the whitelist and the blacklist are empty, we should skip the loop completely.
If the user is using the filtering, then we are iterating over the list of frames 3 times; once to apply the regex, once to sort, and once to build up the current frames. It seems to me that we should be able to get away with only iterating once. That is, we have a loop, and in the loop body we compare it to the whitelist and blacklist. If it is not to be shown because it is explicitly in the blacklist, or it is not in the whitelist (or it is empty), we do nothing more. If we are going to show it, then we create/update the frame for it, and insert it into current_frames. Since current_frames is a set anyway (which is sorted: https://en.cppreference.com/w/cpp/container/set), there is no need for a separate sort step. We can do a similar thing for the case where the whitelist and blacklist are empty, and skip the additional sort step.
At least, that's how it seems to me. Does that make sense to you? Am I possibly missing something with the separate sort and "insert into sorted set" step?
There was a problem hiding this comment.
what do you think @clalancette ? 246e0d7
rviz_default_plugins/src/rviz_default_plugins/displays/tf/tf_display.cpp
Show resolved
Hide resolved
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
|
friendly ping @clalancette |
|
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
|
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
|
|
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-jazzy-jalisco-released/37862/1 |
related with this PR in RViz for ROS 1