Adds a MeshCache to _msgsToAttachedBody to prevent duplicate attached collision objects being stored in FCLShapeCache#3518
Conversation
…l needs documenting and testing.
|
Thanks for helping in improving MoveIt and open source robotics! |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3518 +/- ##
==========================================
+ Coverage 61.81% 61.89% +0.08%
==========================================
Files 385 387 +2
Lines 34121 34176 +55
==========================================
+ Hits 21088 21149 +61
+ Misses 13033 13027 -6
☔ View full report in Codecov by Sentry. |
|
I really recommend you to install pre-commit, saves some round trips with the ci. https://moveit.ros.org/documentation/contributing/code/ |
Yeah, sorry, my editor had the wrong clang-format loaded and kept undoing my changes. I think it's fixed now. |
|
Some open questions I have on this:
|
|
Very nice PR, thanks!
Did you measure the overhead? Is there a size where it does not help to use the cache? I would suspect the turn-even << 1MB?
Maybe calculating hashes directly on
So this basically boils down to the question if hash collisions are likely (enough) to warrant the storage costs. Maybe storing something like number of vertices and/or edges would be sufficient? |
I haven't measured any overhead, and I'm not sure if my use cases are representative of wider use. I think the use case for the minimum size is if someone has a few large collision objects and many small ones, then the idea is to not cache the small objects to avoid kicking the large objects out of the cache. But I guess that use case is relatively specific, so the min size should default to zero? For the maximum cache size, I guess it depends on how much RAM the user's computer has.
Makes sense. I'll try that and see how it goes.
I was assuming hash collisions were very unlikely, but that the cache returning the wrong shape would be critically bad. |
…faults min_size_to_cache to 0 and adds a todo.
I've changed the default I looked into changing the hash to use the raw message type rather than the string representation, but I think this is just going to add a lot of code complexity for little gain. |
|
It looks like there's a problem using this PR with OMPL: When two attached collision objects in the scene have the same mesh (and so their shape::Mesh* pointers are aliased by the MeshCache in this PR), the OMPL planner hangs forever on Before I investigate further, I'd like to know if either:
|
|
@rhaschke could I get your thoughts on this PR and the issue with OMPL when you have the time? I'm keen to get this ready to merge, but not sure how to proceed. |
|
This is a bit surprising as I couldn't spot any obvious new locks/mutexes and the cache is thread-local. Could you provide a backtrace from when it hangs? |
Sure, I'll give that a go.
I don't think it's my code that's hanging, but rather that OMPL is hanging as a result of the aliased meshes. |
|
I am struggling to attach gdb to the move_group in our system due to our docker and dependency management setup. I have passed this over to @rr-tom-noble who knows more about these systems. We should have a backtrace for you soon. While trying to attach gdb, I did notice that it is not a full hang. OMPL just goes a lot slower than it should. I'm wondering if it is some kind of cache thrashing on the OMPL end. Hopefully we'll know more shortly. |
|
From which I think the relevant section is |
|
I'm not quite sure what to make of that. It looks like these changes make OMPL's use of fcl::BVHModel much worse than before? |
|
I wonder if this is the wrong solution to the problem, and if something a bit more targeted to FCL would be more appropriate? If I can get FCL to hash the meshes in its cache, I could change that cache to identify identical meshes which don't share a pointer, and change that cache to look more like the cache I have implemented here. That would hopefully solve the problem without so many possible downstream consequences, and not introduce a second cache where a second cache isn't really needed. I'll probably look into doing this sometime next week. |
I missed this particular point (two) until now. The cache used in the FCL collision checker plugin (not actually part of FCL but MoveIt) assumes there's at most one attached and one non-attached instance of a certain mesh. Your use-case breaks this assumption. Not sure how to fix/extend this actually ... |
|
Since this PR introduces downstream problems, and the performance issues appear to be fixed by #3590, I'm going to close it. |
Description
Fixes #3515
Previously, when planning a sequence motion, the attached collision objects would be converted to separate shapes::Mesh* for each waypoint along the path. Then each instance would be come a separate fcl::BVHModel instance in the FCLShapeCache, leading to large computational costs and memory usage when using large attached collision objects.
This PR adds caching of shapes constructed from meshes to _msgsToAttachedBody in conversions.cpp. This means that the shapes::Mesh* pointers passed into the FCLShapeCache are correctly aliased, and fcl::BVHModels do not need to be regenerated.
The mesh cache has an (approximate) maximum memory usage, and evicts cache entries using a last-used-first-out strategy when this memory usage is exceeded.
Checklist