Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add ROS output for the computer vision #10
Add ROS output for the computer vision #10
Changes from 6 commits
07a8efd
872aa4e
46a01bf
439483e
0da2cac
751bb0b
f5a972f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 you can greatly simplify, depending on your version of c++. I'd assume at least 11. Otherwise something is quite off here.
With c++17 you can destructure the key-value pair (
item
) to make it nicer,for(const auto& [key, _] : led_rows) ...
.With c++20 you could initialize from a view instead:
c++23 will probably further simplify :) (
.to
). At least they are getting there :D.Generally this pattern has a quite sub-optimal performance because it creates the same vector on each call. Not a sign. problem here, but you should know.
To address that I'd use a static variable and IIFE to initialize it. Here a c++20 compatible example but you can do even with c++11.
(IMPORTANT: this makes only sense if you change the return type to a const ref at the same time)
Another performance sub-optimality is the needless copy of a std::string. A vector of
string_view
s could make that better since c++17.But no need to do that -- optimization is important to have a good basic feeling for but should usually be done with care -- if it make the code more complicated because that is usually the more problematic cost foremost because it leads to bugs!
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.
You can still use a for-: loop saving you the
rows
variable: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 lookup
led_parser_->getLedRow(name)
indicates to me that actually, you should directly iterate overled_rows
map's items (ideally with c++17 structured binding) to avoid the ugly.first
and.second
..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.
instead of namespacing the topic name, its probably better to just name them image and led_state and let the namespacing be done via roslaunch
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.
"Image" is ambiguous with topics from image_undistort, so I leave it like this