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
costmap_cspace: add costmap_3d_layers library #688
Conversation
processMapOverlay now requires boolean arguement to toggle updateChainEntry().
This reverts commit 9289aca.
[388] PASSED on melodicAll tests passed
[388] PASSED on noeticAll tests passed
|
Codecov Report
@@ Coverage Diff @@
## master #688 +/- ##
=======================================
Coverage 90.96% 90.96%
=======================================
Files 61 61
Lines 4593 4593
=======================================
Hits 4178 4178
Misses 415 415 |
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.
Thank you for the PR.
Some general comments:
-
Please avoid changing white spaces unrelated to this PR.
If you think it's better to change the white spacing, please open another PR. -
Some of the code comments seem describing PR and not describing the code itself.
For example,Add something
explains something is added by this PR, but it's not very meaningful after it's been merged.
Please make sure code comments describes the code itself, and put comments describing the PR as PR description or self-review comments.
costmap_cspace/CMakeLists.txt
Outdated
|
||
# Add library for costmap_3d_layers.cpp | ||
add_library(costmap_3d_layers_lib src/costmap_3d_layers.cpp) |
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 think _lib
is not very common for linux library file name and this will result lib/libcostmap_3d_layers_lib.so
.
So, costmap_3d_layers
would be better if there's no name conflict.
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.
Thank you for your suggestions. I will make changes and PR the corrections.
@@ -346,7 +346,7 @@ class Costmap3dLayerBase | |||
0, 0, 0, map_->info.width, map_->info.height, map_->info.angle, | |||
base_map->header.stamp)); | |||
} | |||
void processMapOverlay(const nav_msgs::OccupancyGrid::ConstPtr& msg) | |||
void processMapOverlay(const nav_msgs::OccupancyGrid::ConstPtr& msg, const bool update_chain_entry) |
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.
Is it an intended change for this PR?
[390] FAILED on melodic
[390] FAILED on noetic
|
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.
Mistakenly pushed changes on wrong repositiory.
3898fc3
to
ed5f072
Compare
[389] PASSED on melodicAll tests passed
[389] PASSED on noeticAll tests passed
|
[393] PASSED on melodicAll tests passed
[393] PASSED on noeticAll tests passed
|
costmap_cspace/package.xml
Outdated
@@ -24,6 +24,6 @@ | |||
|
|||
<depend>costmap_cspace_msgs</depend> | |||
<depend>neonavigation_common</depend> | |||
|
|||
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.
Please remove this whitespace
[395] PASSED on melodicAll tests passed
[395] PASSED on noeticAll tests passed
|
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.
LGTM
No description provided.