-
Notifications
You must be signed in to change notification settings - Fork 41
Robot multigeometry #258
base: master
Are you sure you want to change the base?
Robot multigeometry #258
Conversation
5423b68 to
5464e40
Compare
5d4413a to
bdc945e
Compare
mxgrey
left a comment
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.
This looks like a really great proof of concept. When we start to contend with more precise and demanding use cases, this feature will be very valuable.
Before we can merge this PR, I think we'll need to refine some details of the public API, implementation, and documentation. The time it will take to make these changes means we'll have to manually migrate this PR over to the new repo.
Here are some notes on the public API:
- It seems that only the footprint supports offset geometries. This is problematic because the offset geometry is more relevant for defining the vicinity. Robots tend to be more bothered by seeing obstacles in front of themselves rather than behind or to the sides, so being able to add a forward-offset shape to the vicinity would very helpful.
- I think we can do a cleaner design than using the
Profileclass as an aggregator of offset shapes. I would recommend that we instead define the following classes: OffsetShapeinheritsShapeand ties together an offset and aShapePtrShapeGroupinheritsShapeand ties together a set ofShapePtrobjectsConvexOffsetShapeinheritsOffsetShapebut is specialized forConvexShapePtrConvexShapeGroupinheritsShapeGroupbut is specialized forConvexShapePtrFinalOffsetShape,FinalShapeGroup,FinalConvexOffsetShape, andFinalConvexShapeGroupwould be the "finalized" versions of the above classes. When generating these objects, we would flatten any nested group hierarchies that may have formed, applying offsets as necessary to make sure that the net offset of each leaf shape has the correct value.- With the above approach, we can leave the
Profileclass the way it is, and allow users to simply pass aFinalConvexShapeGroupin for the footprint or the vicinity.
For the implementation:
- I'm not completely sure, but it looks like the implementation only works for circle-circle collisions right now. I'd be very reluctant to merge this feature if the implementation can't be extended to other types of convex shapes.
- Let's explore the feasibility of adding at least one more convex shape to this, like a capsule. It may be a good idea to first introduce capsules separately as a different PR, and just use the built-in FCL methods for that.
For the documentation:
- It would be good to have some kind of write-up of the performance characteristics of the sidecar. E.g. when you add a sidecar to your profile, how does it impact the speed of conflict detection?
- It would also be good to have a write-up on reliability. Does this implementation give any false positives or false negatives for conflict detection? If so, what are the conditions for those false results? Are there any strategies that users can leverage to mitigate those conditions?
|
Thanks for the review @mxgrey. Off the top of my head:
That's correct. It's easy to extend it to other convex by replacing the distance function. It's on my todo list.
There's a problem in that the FCL implementation of capsules assumes the same radius for both endpoints. This essentially means if we have 2 bodies of different sizes and we want to merge them into a capsule we'll have to take the larger sized radius, using up a good amount of space. Let me know if you think that's fine and I'll put it in. |
This actually agrees with the definition of "capsule" that I"m familiar with: the volume of a ball swept along a line segment, with uniform radius from end to end. The more complex convex hull you're thinking of would be a very cool thing to support as well, but I think the ordinary uniform capsule would be a great thing to start with. |
535735b to
23a897c
Compare
After some more digging, some blockers for using the built-in FCL methods:
If we continue on this path, we'd have to creatively configure the axis of the trajectories and offset the trajectories to align the circles. I'll continue on with adapting the current algorithm for boxes, but let me know if there's still ways around that can be done that you want explored. |
With the purpose of minimizing the amount of code/objects, I'm doing this instead:
Let me know if there's anything of concern. |



This PR provides support for adding circular bodies with an offset from the main robot. The current limit is set to 1 additional body for performance reasons. The main collision algorithm code lies in
rmf_traffic/SplineCCD.cpp'collide_seperable_circles. It computes a distance vector to cover along the spline, then uses bilateral advancement to find roots.see
rmf_traffic/Profile.hppfor public api changesNotable points of contention, let me know your thoughts on any of these:
WRT
check_collision, if there are no extra geometry for the profile, we revert to the old FCL method.Edit*: We could have the new algorithm completely supercede the old method but would entail that we drop our Box tests (since this algorithm uses only circles atm). Also, the old FCL method is faster, check the rmf_planner_viz scenario Add conflict tests #8
Tolerance: Set to
0.1in terms of distance. We might want to allow adjustment of these tolerance values elsewhere in future. Also, the FCL method seems to use a time of contact error tolerance as seen hereSafety checks: There is another limit based on the number of distance checks used in the algorithm, currently set to
120. This is to prevent infinite loops in runtime caused by a small tolerance values.Profile api: Should we allow the extra geometry to be named? Y/N?
Vicinity shapes: We still use only one shape for the vicinity. Given the ability to add extra shapes to the footprint, we can automatically calculate the vicinity to never be smaller than the combined geometry. Problem is how do we deal with custom vicinity geometry? Is it time to enforce a circular vicinity geometry instead? Another option is to let users specify the vicinity shape along with the extra footprint geometry, incurring a performance penalty and extra maintainence cost. Yet another option is to leave the current implementation be, and let users maintain the single vicinity shape themshelves.
Other:
test_Conflictdetailing a problem with differential splines generated from splines with no motions. It's not scenario that's likely to occur IRL so we can put it as low priority for now