-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 Vector Object server #4680
base: main
Are you sure you want to change the base?
Add Vector Object server #4680
Conversation
Signed-off-by: Alexey Merzlyakov <[email protected]> Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alexey Merzlyakov <[email protected]>
Signed-off-by: Alexey Merzlyakov <[email protected]> Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alexey Merzlyakov <[email protected]> Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alexey Merzlyakov <[email protected]> Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alexey Merzlyakov <[email protected]>
Signed-off-by: Alexey Merzlyakov <[email protected]> Signed-off-by: Alberto Tudela <[email protected]>
Co-authored-by: Steve Macenski <[email protected]> Signed-off-by: Alexey Merzlyakov <[email protected]> Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alexey Merzlyakov <[email protected]> Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alexey Merzlyakov <[email protected]> Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alexey Merzlyakov <[email protected]> Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alexey Merzlyakov <[email protected]> Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alexey Merzlyakov <[email protected]> Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alexey Merzlyakov <[email protected]> Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alexey Merzlyakov <[email protected]> Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alexey Merzlyakov <[email protected]> Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alexey Merzlyakov <[email protected]> Signed-off-by: Alberto Tudela <[email protected]>
* Corrected headers * Functions ordering * Comment fixes Signed-off-by: Alexey Merzlyakov <[email protected]> Signed-off-by: Alberto Tudela <[email protected]>
* Correct licensing years * Fix Vector Object server dependencies * Funcion rename for better readability * Improve/fix comments Signed-off-by: Alexey Merzlyakov <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Hi @SteveMacenski, if you have a moment, could you look at this and let me know what changes are needed. Thanks very much! |
I took a look at about ~2/3 of it this afternoon. I didn't quite get to all of it, so I'll finish my review next time I get a chance :-) In glancing through the old PR, I came up on this comment #3930 (comment) -- what do you think? |
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.
Done! Largely nitpicks :-) Can you go through the annotations for test coverage where missing and fill in a few of the gaps that are relatively easy to do?
'container_name', default_value='nav2_container', | ||
description='the name of conatiner that nodes will load in if use composition') | ||
|
||
configured_params = ParameterFile( |
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.
Check this against main
: is this still how we handle this? I think this may have changed
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.
I checked and it's the same in other launch files in main
.
PushROSNamespace( | ||
condition=IfCondition(NotEqualsSubstitution(LaunchConfiguration('namespace'), '')), | ||
namespace=namespace), | ||
Node( |
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.
Put the lifecycle manager last so it doesn't try to bring servers up that aren't yet initialized. Ditto in the composition version below
|
||
bool VectorObjectServer::obtainParams() | ||
{ | ||
// Main ROS-parameters |
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.
Usually we do a auto node = shared_from_this()
so that the node
is used in all the calls below (its more natural looking)
shared_from_this(), "shapes", std::vector<std::string>()).as_string_array(); | ||
for (std::string shape_name : shape_names) { | ||
std::string shape_type; | ||
|
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.
Remove line
} | ||
shapes_.push_back(circle); | ||
} else { | ||
RCLCPP_ERROR(get_logger(), "Please specify correct shape %s type", shape_name.c_str()); |
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.
In the log, give the options so they can fix it fast
|
||
for (auto shape : shapes_) { | ||
shape->getBoundaries(min_p_x, min_p_y, max_p_x, max_p_y); | ||
if (min_p_x < min_x) { |
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.
std::min
and std::max
would be more concise
if (!map_) { | ||
map_ = std::make_shared<nav_msgs::msg::OccupancyGrid>(); | ||
} | ||
if ( |
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.
Add space between if statements
|
||
void VectorObjectServer::publishMap() | ||
{ | ||
if (map_) { |
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.
Check also if map_pub_
's number of subscribers is > 0
to bother with the operation at all
publishMap(); | ||
} | ||
|
||
void VectorObjectServer::switchMapUpdate() |
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.
I am a little baffled on this method.
Do you have an idea why we create a map_timer_
for each object who's frame ID doesn't match the current frame ID? It seems to override each other. We then instantly cancel it if it exists too... and processMap
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.
I'm not exactly sure. I read this in the doc PR:
- If at least one of the shape is set in different than map's frame, dynamic update model to be enabled: this shape can move over the time, output map will be published dynamically with a given rate.
- If all shapes are set in the same as map frame, map is being published/updated once: during Vector Object server startup and per each shape changing call (
AddShapes.srv
orRemoveShapes.srv
).
max_y = std::numeric_limits<double>::lowest(); | ||
|
||
for (auto point : polygon_->points) { | ||
if (point.x < min_x) { |
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.
std min/max
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Basic Info
Description of contribution in a few bullet points
Just the commits from #3930
Description of documentation updates required from your changes
Future work that may be required in bullet points
For Maintainers: