[ROS2] Porting bodies::Body::computeBoundingBox changes from noetic to ROS2 #239
Conversation
Co-authored-by: Tyler <[email protected]>
There was a problem hiding this comment.
The PR looks good to me in general.
It needs a few changes I left as review comments. Most importantly, getting rid of the code complications needed to support FCL 0.5 .
Note for maintainers: This PR contains the original #210 PR adding OBBs as well as all the later fixups required to make it work.
Also, this PR breaks ABI. I'm not sure if that is a problem or not. If it is a problem, we can implement the same temporary workarounds that were done in b00b1e5 .
| @@ -240,6 +241,10 @@ class Body | |||
| pose. Scaling and padding are accounted for. */ | |||
| virtual void computeBoundingBox(AABB& bbox) const = 0; | |||
|
|
|||
| /** \brief Compute the oriented bounding box for the body, in its current | |||
| pose. Scaling and padding are accounted for. */ | |||
| virtual void computeBoundingBox(OBB& bbox) const = 0; | |||
There was a problem hiding this comment.
This will break ABI (the virtual keyword). Not sure what is the current policy regarding ABI breakages.
Co-authored-by: Martin Pecka <[email protected]>
pr review fixes
|
I've removed to and from FCL and the related 0.5 checks |
CMakeLists.txt
Outdated
| @@ -66,6 +67,7 @@ set(THIS_PACKAGE_EXPORT_DEPENDS | |||
| resource_retriever | |||
| shape_msgs | |||
| visualization_msgs | |||
| fcl | |||
There was a problem hiding this comment.
I don't think fcl needs to be exported as a dependency. It is only used from .cpp files and not from the .h files. So it should only be build and export dependency, not build_export dependency.
There was a problem hiding this comment.
Im still newer to cmake, in that case would I just remove it from this export set and the rest can remain the same?
| @@ -30,14 +30,16 @@ | |||
|
|
|||
| #include <geometric_shapes/aabb.h> | |||
|
|
|||
| #include <fcl/config.h> | |||
| #include <fcl/geometry/shape/utility.h> | |||
|
|
|||
There was a problem hiding this comment.
These includes now seem irrelevant.
There was a problem hiding this comment.
With switching to the correct ifdef as you mentioned below I believe that #include <fcl/geometry/shape/utility.h>
is still required?
There was a problem hiding this comment.
Right, some fcl include is still required. This seems good.
|
@rhaschke @henningkayser Is support for Foxy and Galactic still required? If so, their underlying Ubuntu 20.04 still distributes FCL 0.5, which would mean this PR would need to retain all the FCL 0.5 checks. |
we just merged support for Jazzy and dropped Foxy and Galactic. Could you update this branch to rerun CI @spelletier1996? |
|
@spelletier1996 Please, do the one update of package.xml I asked. With that done, I hope this PR would be ready for merging. |
|
I'm not sure how available @spelletier1996 is to make changes, so I went ahead and modified the If the linked rosdistro PR for libfcl gets merged let me know and I can edit this again to remove the -dev |
This is the ROS2 port of the changes from #210
Needed to eventually port robot_body_filter to ROS2
We initially tried to git cherry-pick directly but ran into issues hence the extra commits.
Adds fcl to the build, the obb files, computeBoundingBox (obb version), mergeBoundingBoxesApprox, and related boundingbox tests