-
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 ability to publish layers of the costmap #3254
Conversation
@jwallace42, please properly fill in PR template in the future. @SteveMacenski, use this instead.
|
Codecov ReportBase: 82.29% // Head: 82.15% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #3254 +/- ##
==========================================
- Coverage 82.29% 82.15% -0.14%
==========================================
Files 341 341
Lines 15537 15554 +17
==========================================
- Hits 12786 12779 -7
- Misses 2751 2775 +24
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
From my perspective it seems to be OK. If @SteveMacenski are also OK with it, we could merge.
Can you play a little bit of the motivation for this work? Is this based on a need or perceived need? |
I am trying to increase the ability of the behavior server. Some types of behavior recoveries may want access to a particular layer. The other goal was to enhance the ability to debug layers.
I really think all of the layers should be publishable but this doesn't see to be currently supported. See the following conversation.
These are lazy publishers so it should only be publishing if there is a active sub. |
That seems abusive, but OK. Is that layer required to be in both places? Would it not be possible to have that layer just in the behavior in the first place and not in the costmap writ large? Does the behavior really need the full costmap layer, or just some service to perform an action or series of actions upon it?
Ah, you can make everyone's lives easier if you just try to dynamic cast the pointer. If that conversion cannot be made, its a nullpointer, so you can check if its a nullpointer before populating it into the vector of publishers. That way no new API is required. Blah blah blah, unit tests :-) Something to create a costmap with some dummy layer with some random values, then subscribe to that dummy layer and make sure it gets the message and checks the dummy values / size are right. |
Can you expand what you mean here? I assume that you are suggesting to create a another instance of the costmap layer in the behavior?
So, a service hosted by the costmap would also work. The only issue here that costmap_2d does not have any plugin interface setup. The other thing to consider is the amount of time that service would get called by the behavior. (I would assume that calling the service is probably more efficient than listening to a layer and doing some checks).
Will do :).
Sounds good. |
If you're trying to expose Layer X to to somewhere else from the costmap, I'm asking if the costmap itself actually needs Layer X or you just have Layer X in the costmap because its just where all the other data is. So, do you need Layer X both in the costmap for the planner/controller server, or do you just need Layer X in this 3rd party application? More succinctly: Do you need this data in both places or just in the 3rd party application? Its easy to create an independent costmap object to just do Layer X without much overhead -- arguably better designed and definitely lower latency especially for large costmap sizes. But only if its only needed in that 1 place. I don't know what you mean by "does not have any plugin interface setup" --> there are initialization functions https://github.com/ros-planning/navigation2/blob/main/nav2_costmap_2d/include/nav2_costmap_2d/obstacle_layer.hpp#L86
This is perhaps where a private conversation about what more specifically we're talking about here might be beneficial. There's not an abstract best answer, this is really going to depend on what we're talking about. Either way, it is sensible for at least debugging to be able to visualize each layer so I think this PR makes sense. I'd just like you to do some benchmarking to sanity check things:
CI for the PR failed from linting 🙃 |
Also unit tests for this would be strictly required, this is enough of a change that we need to know if it breaks at some point in the future from another change |
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.
Unit tests / rough benchmark numbers for this are still necessary, but otherwise LGTM
I think this is just waiting on tests |
This pull request is in conflict. Could you fix it @jwallace42? |
} | ||
|
||
protected: | ||
std::shared_ptr<TestCostmap2dPublisher> costmap_publisher_; |
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.
Move the costmap 2d ros object into the TestNode. Its quite confusing to have it in the publisher fixture. I think that separates your intent much more cleanly.
}; | ||
RclCppFixture g_rclcppfixture; | ||
|
||
class TestCostmap2dPublisher : public nav2_util::LifecycleNode |
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.
Isn't this a subscriber mostly?
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.
Rename to TestCostmap2DSubscriber
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.
We are testing the that the costmap_2d_ros_ object publishes the layer of the costmap. Maybe a better name would be TestCostmap2DLayerPub
?
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.
Its not publishing anything though, especially after costmap_ros_
moves out of the object. It only contains a subscription.
The same question still apply: Who's publishing to the static layer in the first place to populate it? I think you need to create a publisher of the occupancy grid with some values to check for. Then, check that the costmap output from the layer properly reflects that. Otherwise, you don't really know what it is you're receiving or if that's actually representative of the contents of a layer. For instance, publish to the static layer a 100x100 costmap with a 10x10 block interior to it as occupied. Then check when you receive the output of the layer's raw published state that its free except in that block. That way we know it published the contents correctly rather than publish only initialized values out. Why this is important is because a unit test like that could have actually turned us onto the callback group processing issues from #3014 that we see that the contents are wrong and fully empty. |
The static layer is using the map here https://github.com/ros-planning/navigation2/tree/main/nav2_costmap_2d/test/map. This is published by the map_server. That is why I don't need to create a publisher for static layer. I will check the complete TenbyTen map instead of a row to verify that map is indeed a match. |
Ah I see, I missed that you had a launch file being used here. Last 2 things then are in those comments of just shuffling around some code for clarity + getting CI to turn over (conflict) |
I just updated the tests. I think you will like it a lot more :). Currently waiting for the CI to turn over. |
It looks like you deleted or commented out a number of unrelated things in the costmap tests - I think that needs to be reverted. |
Awesome! As a post mortem on this one since this took ~6 week to merge in what should have been small - maybe it would have been good if we got on a call. It seems alot of our missing each other wasn't as much on the feature as the unit tests to cover it which probably could have been resolved in a 15 minute call 😆 I'll keep that in mind in the future |
This reverts commit 9538ada.
@jwallace42 can you submit the docs PR to Navigation.ros.org? |
|
* publish layers of costmap * lint fix * lint round 2 :) * code review * remove isPublishable * lint * test running * rough structure complete * completed test * lint * code review * CI * CI * linting * completed pub test * CostmapLayer::matchSize may be executed concurrently (ros-navigation#3250) * CostmapLayer::matchSize() add a mutex * Fix typo (ros-navigation#3262) * Adding new Nav2 Smoother: Savitzky-Golay Smoother (ros-navigation#3264) * initial prototype of the Savitzky Golay Filter Path Smoother * fixed indexing issue - tested working * updates for filter * adding unit tests for SG-filter smoother * adding lifecycle transitions * refactoring RPP a bit for cleanliness on way to ROSCon (ros-navigation#3265) * refactor for RPP on way to ROSCon * fixing header * fixing header * fixing header * fix edge cases test samplings * linting * exceptions for compute path through poses (ros-navigation#3248) * exceptions for compute path through poses * lint fix * code review * code review Co-authored-by: Joshua Wallace <josho.wallace.com> * Reclaim Our CI Coverage from the Lords of Painful Subtle Regressions ⚔️⚔️⚔️ (ros-navigation#3266) * test waypoint follower with composition off for logging * adding no composition to all system tests * Added Line Iterator (ros-navigation#3197) * Added Line Iterator * Updated Line Iterator to a new iteration method * Added the resolution as a parameter/ fixed linting * Added the resolution as a parameter/ fixed linting * Added unittests for the line iterator * Added unittests based on "unittest" package * Fixed __init__.py and rephrased some docstrings * Fixed linting errors * Fixed Linting Errors * Added some unittests and removed some methods * Dummy commit for CircleCI Issue Co-authored-by: Afif Swaidan <[email protected]> * Use SetParameter Launch API to set the yaml filename for map_server (ros-navigation#3174) * implement launch priority for the mapserver parameter yaml_filename minor fix fix commit reincluded rewritten function comment remaining lines for yaml_filename removed default_value issue with composable node alternative soltion to the condition param not working in composable node remove unused import remove comments and reorder composablenode execution fixing commit fixing format fixing lint Update nav2_bringup/params/nav2_params.yaml Co-authored-by: Steve Macenski <[email protected]> * state new ros-rolling release changes and deprecation Co-authored-by: Steve Macenski <[email protected]> * adding reconfigure test to thetastar (ros-navigation#3275) * Check for range_max of laserscan in updateFilter to avoid a implicit overflow crash. (ros-navigation#3276) * Update amcl_node.cpp * fit the code style * fit code style * BT Service Node to throw if service was not available in time (ros-navigation#3256) * throw if service server wasn't available in time mimic the behavior of the bt action node constructor * throw if action unavailable in bt cancel action * use chrono literals namespace * fix linting errors * fix code style divergence * remove exec_depend on behaviortree_cpp_v3 (ros-navigation#3279) * add parameterized refinement recursion numbers in Smac Planner Smoother and Simple Smoother (ros-navigation#3284) * add parameterized refinement recursion numbers * fix tests * Add allow_unknown parameter to theta star planner (ros-navigation#3286) * Add allow unknown parameter to theta star planner * Add allow unknown parameter to tests * missing comma * Change cost of unknown tiles * Uncrustify * Include test cases waypoint follwer (ros-navigation#3288) * WIP * included missed_waypoing check * finished inclding test * fix format * return default sleep value * Dynamically changing polygons support (ros-navigation#3245) * Add Collision Monitor polygon topics subscription * Add the support of polygons published in different frame * Internal review * Fix working with polygons visualization * Update nav2_collision_monitor/README.md Co-authored-by: Steve Macenski <[email protected]> * Move getTransform to nav2_util * Fix misprint * Meet remaining review items: * Update polygon params handling logic * Warn if polygon shape was not set * Publish with ownership movement * Correct polygons_test.cpp parameters handling logic * Adjust README for dynamic polygons logic update Co-authored-by: Steve Macenski <[email protected]> * adding getCostScalingFactor (ros-navigation#3290) * Implemented smoother selector bt node (ros-navigation#3283) * Implemented smoother selector bt node Signed-off-by: Owen Hooper <[email protected]> * updated copyright in modified file Signed-off-by: Owen Hooper <[email protected]> Signed-off-by: Owen Hooper <[email protected]> * Pipe error codes (ros-navigation#3251) * issue with finding key * passed up codes to bt_navigator * lint fix * updates * adding error_code_id back in * error codes names in params * bump error codes * lint * spelling * test fix * update behavior trees * cleanup * Update bt_action_server_impl.hpp * code review * lint * code review * log fix * error code for waypoint follower * clean up * remove waypoint error test, too flaky on CI * lint and code review * rough imp for waypoint changes * lint * code review * build fix * clean up * revert * space * remove * try to make github happ * stop gap * loading in param file * working tests :) * lint * fixed cmake * lint * lint * trigger build * added invalid plugin error * added test for piping up error codes * clean up * test waypoint follower * only launch what is needed * waypoint test * revert lines for robot navigator * fix test * waypoint test * switched to uint16 * clean up * code review * todo to note * lint * remove comment * Update nav2_behavior_tree/include/nav2_behavior_tree/bt_action_server_impl.hpp Co-authored-by: Steve Macenski <[email protected]> * rename error_codes * error code for navigate to pose * error codes for navigate through poses. * error codes for navigate through poses * message update for waypoint follower * rename to error code * update node xml Co-authored-by: Joshua Wallace <josho.wallace.com> Co-authored-by: Steve Macenski <[email protected]> * Solve bug when CostmapInfoServer is reactivated (ros-navigation#3292) * Solve bug when CostmapInfoServer is reactivated * Smoothness metrics update (ros-navigation#3294) * Update metrics for path smoothness * Support Savitzky-Golay smoother * preempt/cancel test for time behavior, spin pluguin (ros-navigation#3301) * include preempt/cancel test for time behavior, spin pluguin * linting * fix bug in code * lint fix * clean up test * lint * cleaned up test * update * revert Cmake Signed-off-by: Owen Hooper <[email protected]> Co-authored-by: Joshua Wallace <josho.wallace.com> Co-authored-by: Hao-Xuan Song <[email protected]> Co-authored-by: jaeminSHIN <[email protected]> Co-authored-by: Steve Macenski <[email protected]> Co-authored-by: Afif Swaidan <[email protected]> Co-authored-by: Afif Swaidan <[email protected]> Co-authored-by: Stevedan Ogochukwu Omodolor <[email protected]> Co-authored-by: Pradheep Krishna <[email protected]> Co-authored-by: Erwin Lejeune <[email protected]> Co-authored-by: Adam Aposhian <[email protected]> Co-authored-by: Pedro Alejandro González <[email protected]> Co-authored-by: Alexey Merzlyakov <[email protected]> Co-authored-by: Owen Hooper <[email protected]> Co-authored-by: MartiBolet <[email protected]>
…3254)" (ros-navigation#3319) This reverts commit 9538ada.
* publish layers of costmap * lint fix * lint round 2 :) * code review * remove isPublishable * lint * test running * rough structure complete * completed test * lint * code review * CI * CI * linting * completed pub test * CostmapLayer::matchSize may be executed concurrently (ros-navigation#3250) * CostmapLayer::matchSize() add a mutex * Fix typo (ros-navigation#3262) * Adding new Nav2 Smoother: Savitzky-Golay Smoother (ros-navigation#3264) * initial prototype of the Savitzky Golay Filter Path Smoother * fixed indexing issue - tested working * updates for filter * adding unit tests for SG-filter smoother * adding lifecycle transitions * refactoring RPP a bit for cleanliness on way to ROSCon (ros-navigation#3265) * refactor for RPP on way to ROSCon * fixing header * fixing header * fixing header * fix edge cases test samplings * linting * exceptions for compute path through poses (ros-navigation#3248) * exceptions for compute path through poses * lint fix * code review * code review Co-authored-by: Joshua Wallace <josho.wallace.com> * Reclaim Our CI Coverage from the Lords of Painful Subtle Regressions ⚔️⚔️⚔️ (ros-navigation#3266) * test waypoint follower with composition off for logging * adding no composition to all system tests * Added Line Iterator (ros-navigation#3197) * Added Line Iterator * Updated Line Iterator to a new iteration method * Added the resolution as a parameter/ fixed linting * Added the resolution as a parameter/ fixed linting * Added unittests for the line iterator * Added unittests based on "unittest" package * Fixed __init__.py and rephrased some docstrings * Fixed linting errors * Fixed Linting Errors * Added some unittests and removed some methods * Dummy commit for CircleCI Issue Co-authored-by: Afif Swaidan <[email protected]> * Use SetParameter Launch API to set the yaml filename for map_server (ros-navigation#3174) * implement launch priority for the mapserver parameter yaml_filename minor fix fix commit reincluded rewritten function comment remaining lines for yaml_filename removed default_value issue with composable node alternative soltion to the condition param not working in composable node remove unused import remove comments and reorder composablenode execution fixing commit fixing format fixing lint Update nav2_bringup/params/nav2_params.yaml Co-authored-by: Steve Macenski <[email protected]> * state new ros-rolling release changes and deprecation Co-authored-by: Steve Macenski <[email protected]> * adding reconfigure test to thetastar (ros-navigation#3275) * Check for range_max of laserscan in updateFilter to avoid a implicit overflow crash. (ros-navigation#3276) * Update amcl_node.cpp * fit the code style * fit code style * BT Service Node to throw if service was not available in time (ros-navigation#3256) * throw if service server wasn't available in time mimic the behavior of the bt action node constructor * throw if action unavailable in bt cancel action * use chrono literals namespace * fix linting errors * fix code style divergence * remove exec_depend on behaviortree_cpp_v3 (ros-navigation#3279) * add parameterized refinement recursion numbers in Smac Planner Smoother and Simple Smoother (ros-navigation#3284) * add parameterized refinement recursion numbers * fix tests * Add allow_unknown parameter to theta star planner (ros-navigation#3286) * Add allow unknown parameter to theta star planner * Add allow unknown parameter to tests * missing comma * Change cost of unknown tiles * Uncrustify * Include test cases waypoint follwer (ros-navigation#3288) * WIP * included missed_waypoing check * finished inclding test * fix format * return default sleep value * Dynamically changing polygons support (ros-navigation#3245) * Add Collision Monitor polygon topics subscription * Add the support of polygons published in different frame * Internal review * Fix working with polygons visualization * Update nav2_collision_monitor/README.md Co-authored-by: Steve Macenski <[email protected]> * Move getTransform to nav2_util * Fix misprint * Meet remaining review items: * Update polygon params handling logic * Warn if polygon shape was not set * Publish with ownership movement * Correct polygons_test.cpp parameters handling logic * Adjust README for dynamic polygons logic update Co-authored-by: Steve Macenski <[email protected]> * adding getCostScalingFactor (ros-navigation#3290) * Implemented smoother selector bt node (ros-navigation#3283) * Implemented smoother selector bt node Signed-off-by: Owen Hooper <[email protected]> * updated copyright in modified file Signed-off-by: Owen Hooper <[email protected]> Signed-off-by: Owen Hooper <[email protected]> * Pipe error codes (ros-navigation#3251) * issue with finding key * passed up codes to bt_navigator * lint fix * updates * adding error_code_id back in * error codes names in params * bump error codes * lint * spelling * test fix * update behavior trees * cleanup * Update bt_action_server_impl.hpp * code review * lint * code review * log fix * error code for waypoint follower * clean up * remove waypoint error test, too flaky on CI * lint and code review * rough imp for waypoint changes * lint * code review * build fix * clean up * revert * space * remove * try to make github happ * stop gap * loading in param file * working tests :) * lint * fixed cmake * lint * lint * trigger build * added invalid plugin error * added test for piping up error codes * clean up * test waypoint follower * only launch what is needed * waypoint test * revert lines for robot navigator * fix test * waypoint test * switched to uint16 * clean up * code review * todo to note * lint * remove comment * Update nav2_behavior_tree/include/nav2_behavior_tree/bt_action_server_impl.hpp Co-authored-by: Steve Macenski <[email protected]> * rename error_codes * error code for navigate to pose * error codes for navigate through poses. * error codes for navigate through poses * message update for waypoint follower * rename to error code * update node xml Co-authored-by: Joshua Wallace <josho.wallace.com> Co-authored-by: Steve Macenski <[email protected]> * Solve bug when CostmapInfoServer is reactivated (ros-navigation#3292) * Solve bug when CostmapInfoServer is reactivated * Smoothness metrics update (ros-navigation#3294) * Update metrics for path smoothness * Support Savitzky-Golay smoother * preempt/cancel test for time behavior, spin pluguin (ros-navigation#3301) * include preempt/cancel test for time behavior, spin pluguin * linting * fix bug in code * lint fix * clean up test * lint * cleaned up test * update * revert Cmake Signed-off-by: Owen Hooper <[email protected]> Co-authored-by: Joshua Wallace <josho.wallace.com> Co-authored-by: Hao-Xuan Song <[email protected]> Co-authored-by: jaeminSHIN <[email protected]> Co-authored-by: Steve Macenski <[email protected]> Co-authored-by: Afif Swaidan <[email protected]> Co-authored-by: Afif Swaidan <[email protected]> Co-authored-by: Stevedan Ogochukwu Omodolor <[email protected]> Co-authored-by: Pradheep Krishna <[email protected]> Co-authored-by: Erwin Lejeune <[email protected]> Co-authored-by: Adam Aposhian <[email protected]> Co-authored-by: Pedro Alejandro González <[email protected]> Co-authored-by: Alexey Merzlyakov <[email protected]> Co-authored-by: Owen Hooper <[email protected]> Co-authored-by: MartiBolet <[email protected]>
…3254)" (ros-navigation#3319) This reverts commit 9538ada.
Basic Info
Description of contribution in a few bullet points
Currently, only the accumulated layer is published out for consumption by other nodes. Now, all of the layers are published individually.
Description of documentation updates required from your changes
Future work that may be required in bullet points
For Maintainers: