Add Vector Object server #4680
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? |
| PushROSNamespace( | ||
| condition=IfCondition(NotEqualsSubstitution(LaunchConfiguration('namespace'), '')), | ||
| namespace=namespace), | ||
| Node( |
There was a problem hiding this comment.
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.
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; | ||
|
|
| } | ||
| 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.
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.
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.
Add space between if statements
|
|
||
| void VectorObjectServer::publishMap() | ||
| { | ||
| if (map_) { |
There was a problem hiding this comment.
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.
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.
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.srvorRemoveShapes.srv).
There was a problem hiding this comment.
Oh my bad, there's a return; in there I missed. Its not immediately canceled, the function returns if anything is dynamic -- only gets to the cancel part of everything is static
| max_y = std::numeric_limits<double>::lowest(); | ||
|
|
||
| for (auto point : polygon_->points) { | ||
| if (point.x < min_x) { |
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
I'm conflicting. I prefer a more friendy ID that uses an I added the final tests to fill the missing gaps that were harder to tick and the nitpicks you mentioned. P.S.: The only file without a test is the main.cpp, but I'm not sure how to test it. |
|
@SteveMacenski it's ready for review, when you can |
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: