Skip to content
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 sub-directories for external dependencies of tesseract_collision #641

Conversation

marip8
Copy link
Contributor

@marip8 marip8 commented Oct 13, 2021

This PR divides tesseract_collision into sub-directories based on the optional external dependencies each sub-directory introduces (bullet, FCL, VHACD).

Note: the function createConvexHull in the library tesseract_collision_core currently depends on bullet API, so I left the function signature in core but moved the implementation to the tesseract_collision_bullet library. This means that any code that wants to use createConvexHull (i.e. functions in tesseract_urdf) needs to link against the tesseract_collision_bullet library until we find a better solution for uncoupling the creation of convex hulls from bullet.

@marip8 marip8 requested a review from Levi-Armstrong October 13, 2021 17:03
@Levi-Armstrong
Copy link
Contributor

@marip8 Do you mind if we leave the createConvexHull in core and keep the bullet dependency for now.

@Levi-Armstrong
Copy link
Contributor

Then it may be better to update this function to leverage qhull to break the dependency on bullet for the core.

@marip8
Copy link
Contributor Author

marip8 commented Oct 13, 2021

Do you mind if we leave the createConvexHull in core and keep the bullet dependency for now.

To be clear, the function is still defined in core/common.h so it doesn't break API in other packages; only the definition of the function moved to the bullet sub-directory. The only change required for downstream packages is to link against tesseract_collision_bullet rather than tesseract_collision_core.

There is not an easy way for me to keep the definition in core because it would require all the same commands for finding and linking against bullet that I moved to the bullet subdirectory, which would defeat the purpose of this PR.

I can take a look at qhull to see if we can keep the current behavior without needing the linking hack.

@marip8
Copy link
Contributor Author

marip8 commented Oct 15, 2021

qhull seems relatively difficult to integrate into tesseract_collision at the moment, so we decided to look into using boost to create convex hulls. It has an algorithm, but according to this issue it only works in 2D and not in 3D. I replicated the test case from this issue and was able to confirm that it does not in fact work correctly in 3D.

If we decide to go the qhull route, I would like to do it in a separate PR for more clarity on the changes. @Levi-Armstrong what do you think about this?

@marip8 marip8 force-pushed the update/tesseract-collision-subdirs branch from 7a2b838 to 45fd883 Compare October 15, 2021 23:52
@codecov
Copy link

codecov bot commented Oct 16, 2021

Codecov Report

Merging #641 (a84a870) into master (c8c5b29) will increase coverage by 1.24%.
The diff coverage is 96.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #641      +/-   ##
==========================================
+ Coverage   87.13%   88.38%   +1.24%     
==========================================
  Files         162      178      +16     
  Lines        8786    10557    +1771     
==========================================
+ Hits         7656     9331    +1675     
- Misses       1130     1226      +96     
Impacted Files Coverage Δ
...sseract_collision/bullet/bullet_cast_bvh_manager.h 100.00% <ø> (ø)
...ract_collision/bullet/bullet_cast_simple_manager.h 100.00% <ø> (ø)
...act_collision/bullet/bullet_discrete_bvh_manager.h 100.00% <ø> (ø)
..._collision/bullet/bullet_discrete_simple_manager.h 100.00% <ø> (ø)
.../include/tesseract_collision/bullet/bullet_utils.h 95.29% <ø> (ø)
...llision/bullet/tesseract_collision_configuration.h 100.00% <ø> (ø)
...act_collision/bullet/tesseract_gjk_pair_detector.h 75.00% <ø> (ø)
...t_collision/bullet/src/bullet_cast_bvh_manager.cpp 89.18% <ø> (ø)
...ollision/bullet/src/bullet_cast_simple_manager.cpp 89.90% <ø> (ø)
...llision/bullet/src/bullet_discrete_bvh_manager.cpp 92.80% <ø> (ø)
... and 55 more

@Levi-Armstrong
Copy link
Contributor

How about just moving the createConvexHull to bullet directory. It should only require a few headers update and then the linking is not straight forward.

@Levi-Armstrong
Copy link
Contributor

Also it looks like vhacd depends on bullet also which it is not linking against which is why windows build is failing.

@Levi-Armstrong
Copy link
Contributor

Could also create a simple interface class like the ConvexDecomposition maybe ConvexComputer and then create an implementation in the bullet folder.

@marip8 marip8 force-pushed the update/tesseract-collision-subdirs branch 2 times, most recently from c48a2e7 to 65e018d Compare October 20, 2021 22:09
@marip8 marip8 force-pushed the update/tesseract-collision-subdirs branch from 65e018d to eef3630 Compare October 21, 2021 14:20
@Levi-Armstrong
Copy link
Contributor

Minor fixes to this in PR #649

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants